Uh oh!
There was an error while loading. Please reload this page.
[fix](be) Init thread context on AsyncIO worker threads - #64846
Conversation
hello-stephen
commented
Jun 25, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent BE aborts when RemoteFileSystem::upload() is invoked from a bthread and the call is dispatched via FILESYSTEM_M onto an AsyncIO worker pthread without a ThreadContext, causing LocalFileReader::read_at_impl() (via LIMIT_LOCAL_SCAN_IO) to throw Status::FatalError.
Changes:
- Add
SCOPED_ATTACH_TASK(ExecEnv::GetInstance()->s3_file_buffer_tracker())at the start ofS3FileSystem::upload_impl()to ensure a task/context exists for local reads.
Review Checkpoints (Part 1.3)
- Goal & correctness: Not fully achieved as-is: the new unconditional
SCOPED_ATTACH_TASKcan overwrite/detach an already-attached task whenFILESYSTEM_Mruns inline on pthreads, potentially breaking the caller’s thread context and memory tracking. - Change scope: Small and focused to
upload_impl(), but the current approach needs to be adjusted to be safe in both inline (pthread) and AsyncIO (pthread-from-bthread) execution modes. - Concurrency/threading: Yes—this code explicitly crosses bthread→pthread via
AsyncIO::run_taskand must preserve/avoid clobbering existing task attachment on inline pthread paths. - Parallel paths:
batch_upload_impl()already attaches; single-fileupload_impl()needed equivalent protection, but must be done conditionally to avoid double-attach. - Test coverage: There are existing BE unit tests for
S3FileSystem(including upload), but this change should ideally be covered by a test that callsupload()while a task is already attached, to ensure the thread context remains intact.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
liaoxin01
commented
Jun 25, 2026
/review |
There was a problem hiding this comment.
Summary opinion: I am leaving this as a review comment, not an approval. The only substantiated correctness issue I found is already covered by the existing inline thread on be/src/io/fs/s3_file_system.cpp:329: the new unconditional SCOPED_ATTACH_TASK can run on an already-attached pthread because FILESYSTEM_M executes inline when bthread_self() == 0, and AttachTask is not a safe nested/switching context. I did not add a duplicate inline comment.
Critical checkpoint conclusions:
- Goal/correctness: The patch addresses the bthread-to-AsyncIO missing
ThreadContextcrash path forLocalFileReader::read_at_impl, but it is not fully correct until the existing inline issue is fixed for inline pthread callers. - Scope: The change is small and focused to
S3FileSystem::upload_impl. - Concurrency/lifecycle: This path is thread-context-sensitive.
AttachTaskdetaches at scope exit andThreadContext::attach_taskexpects no existing resource context; snapshot upload provides a concrete already-attached pthread caller. - Config/compatibility/storage format: No new config, FE-BE protocol, function symbol, or storage-format compatibility issue was introduced.
- Parallel paths:
batch_upload_implalready attaches inside its S3 thread-pool callback. HDFS/Broker upload paths were checked and did not produce a distinct changed-line finding for this PR. - Tests: No tests were added or changed. Existing upload tests exercise
upload, but not the attached-context or bthread-to-AsyncIO mode.git diff --check ba778aa4be4218eb4346d15bcd08d53a6db5affa..a520262d5b1b8a64a2bee675951a749f18492670 --was clean. I did not run BE tests in this checkout. - Observability/performance/transactions: No additional observability, performance, transaction, or persistence issue was found beyond the task-context bug already raised.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite, join, aggregate, or semantic-equivalence finding; noted the task-context issue is duplicate of the existing thread.tests-session-config: no new test/session/config/style finding; noted the same issue is already covered andgit diff --checkis clean.- Final convergence round 1: both live subagents replied
NO_NEW_VALUABLE_FINDINGSfor the same current ledger and proposed final comment set.
User focus: no additional user-provided review focus was supplied.
a520262 to
274cc5dCompareliaoxin01
commented
Jun 25, 2026
/review |
There was a problem hiding this comment.
Summary opinion: request changes. The fix is directionally correct for the missing ThreadContext crash path, but the PR does not add focused BE coverage for the exact raw-thread behavior it now relies on.
Critical checkpoint conclusions:
- Goal/correctness: The code changes
LIMIT_LOCAL_SCAN_IOandLIMIT_REMOTE_SCAN_IOto usethread_context_or_null()so raw pthreads skip workload-group throttling instead of throwingFatalError. That matches the reportedFILESYSTEM_M->AsyncIO::run_taskupload path, but the new behavior is untested. - Scope: The code change is small and focused to
be/src/runtime/thread_context.h. - Concurrency/lifecycle: This path is thread-context-sensitive.
FILESYSTEM_Mruns inline on pthread callers and dispatches bthread callers to rawAsyncIOpthreads; the current helper avoids the unsafe unconditionalSCOPED_ATTACH_TASKapproach already raised in the existing thread. - Config/compatibility/storage format: No new config, FE-BE protocol, function symbol, or storage-format compatibility issue was introduced.
- Parallel paths: Both local and remote scan throttle macros were updated. I checked local, S3, HDFS, buffered, and peer-cache reader call sites and found no additional distinct changed-line issue.
- Tests: No test was added. Existing
thread_context_testruns under the BE test runner's main-threadSCOPED_INIT_THREAD_CONTEXT(), so it does not cover a raw thread with no context.git diff --check ba778aa4be4218eb4346d15bcd08d53a6db5affa 274cc5d3cd62345d20cd9d9f216505636d6e626fis clean. - Observability/performance/transactions: No additional issue found; the change does not add persistence or transaction behavior.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite, semantic-equivalence, join, or aggregate candidate; no inline comment from this pass.tests-session-config: proposedTSC-1for missing focused raw-thread coverage; accepted as inline commentC-1.- Final convergence round 1: both live subagents replied
NO_NEW_VALUABLE_FINDINGSfor the same current ledger and proposed final comment set afterC-1was accepted.
User focus: no additional user-provided review focus was supplied.
Uh oh!
There was an error while loading. Please reload this page.
liaoxin01
commented
Jun 26, 2026
run buildall |
FILESYSTEM_M dispatches IO ops (e.g. S3 single-file upload) from a bthread to an AsyncIO worker pthread that is not bound to any task and therefore has no ThreadContext. When the dispatched fn() reads a local file it goes through LocalFileReader::read_at_impl -> LIMIT_LOCAL_SCAN_IO -> thread_context(), which raises Status::FatalError (aborting the BE in debug/ASAN builds) when no ThreadContext exists. Observed as a Cloud P0 coredump on the load error-log S3 upload path: RuntimeState::get_error_log_file_path() -> _s3_error_fs->upload() (from a bthread) -> AsyncIO::run_task() on an AsyncIO worker pthread (no attached task) -> S3FileSystem::upload_impl() -> LocalFileReader::read_at_impl() -> LIMIT_LOCAL_SCAN_IO -> thread_context() -> FatalError -> SIGABRT Initialize an (unattached) ThreadContext at the AsyncIO worker boundary via SCOPED_INIT_THREAD_CONTEXT(), the same pattern used by StorageEngine background threads, so any thread_context() consumer running on the worker is safe.
274cc5d to
f15b39dCompareliaoxin01
commented
Jun 26, 2026
/review |
liaoxin01
commented
Jun 26, 2026
run buildall |
There was a problem hiding this comment.
Summary opinion: no new inline comments for the current head. The current one-file fix moves the ThreadContext initialization to the AsyncIO worker boundary, which matches the reported bthread -> AsyncIO pthread crash path better than the earlier S3-local attach workaround. I am not duplicating the already-known coverage concern from discussion_r3475864070, and the older S3 attach-task concern from discussion_r3473013312 no longer applies to the current changed file.
Critical checkpoint conclusions:
- Goal/correctness: The current patch adds SCOPED_INIT_THREAD_CONTEXT() inside AsyncIO::run_task, so FILESYSTEM_M-dispatched work and LocalFileReader close work on AsyncIO pthreads have a ThreadContext before code such as LIMIT_LOCAL_SCAN_IO calls thread_context(). I found no new correctness issue in the current changed file.
- Scope: The GitHub PR file list contains only be/src/util/async_io.h, and the change is small and focused.
- Concurrency/lifecycle: This is a thread-context boundary. The new scope creates an unattached context only for the worker callback and tears it down after fn() returns; nested SCOPED_ATTACH_TASK paths restore through the existing ThreadMemTrackerMgr snapshot logic. No new lock-order or deadlock issue was found.
- Config/compatibility/storage format: No new config, FE-BE protocol field, function symbol, or storage-format compatibility issue is introduced.
- Parallel paths: FILESYSTEM_M dispatch and LocalFileReader::close both use AsyncIO::run_task, so the worker-boundary fix covers the relevant AsyncIO callers. The previous single S3 upload attach change is absent from this head; batch upload keeps its existing explicit task attach in its own thread-pool callback.
- Tests: No tests were added. The missing focused raw-worker/attached-context coverage concern substantially overlaps the existing inline thread discussion_r3475864070, so I did not submit a duplicate. I did not run BE tests because thirdparty/installed is absent in this checkout. git diff --check for be/src/util/async_io.h on the PR base/head range was clean.
- Observability/performance/transactions/persistence: No additional observability, performance, transaction, or persistence issue was found for this change.
Subagent conclusions:
- optimizer-rewrite: no optimizer/rewrite, semantic-equivalence, join, or aggregate candidate; duplicate context only for the existing S3 attach-task and test-coverage threads.
- tests-session-config: no new tests/session/config/style candidate; the remaining coverage concern is duplicate of discussion_r3475864070.
- Final convergence round 1: both live subagents replied NO_NEW_VALUABLE_FINDINGS for the same current ledger and empty proposed inline comment set after the main merge update.
User focus: no additional user-provided review focus was supplied.
hello-stephen
commented
Jun 26, 2026
TPC-H: Total hot run time: 28632 ms |
hello-stephen
commented
Jun 26, 2026
TPC-DS: Total hot run time: 172059 ms |
hello-stephen
commented
Jun 26, 2026
ClickBench: Total hot run time: 25.13 s |
liaoxin01
commented
Jun 29, 2026
run buildall |
hello-stephen
commented
Jun 29, 2026
TPC-H: Total hot run time: 29306 ms |
hello-stephen
commented
Jun 29, 2026
TPC-DS: Total hot run time: 171897 ms |
hello-stephen
commented
Jun 29, 2026
ClickBench: Total hot run time: 25.45 s |
hello-stephen
commented
Jun 29, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Jun 29, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
## Proposed changes ### Problem `FILESYSTEM_M` dispatches IO ops from a bthread to an `AsyncIO` worker pthread that is **not bound to any task** and therefore has no `ThreadContext`. When the dispatched `fn()` reads a local file, it goes through `LocalFileReader::read_at_impl` -> `LIMIT_LOCAL_SCAN_IO` -> `thread_context()`, which raises `Status::FatalError` (aborting the BE in debug/ASAN builds) when no `ThreadContext` exists. Observed as a Cloud P0 coredump on the load error-log S3 upload path: ``` RuntimeState::get_error_log_file_path() -> _s3_error_fs->upload() (called from a bthread) -> AsyncIO::run_task() on an AsyncIO worker pthread (no attached task) -> S3FileSystem::upload_impl() -> LocalFileReader::read_at_impl() -> LIMIT_LOCAL_SCAN_IO -> thread_context() -> FatalError -> SIGABRT ``` ### Fix Initialize an (unattached) `ThreadContext` at the `AsyncIO` worker boundary via `SCOPED_INIT_THREAD_CONTEXT()`, the same pattern used by StorageEngine background threads. This gives every `thread_context()` consumer running on an AsyncIO worker (memory tracking, `LIMIT_*_SCAN_IO`, ...) a valid context, fixing the crash at its root rather than making individual consumers tolerate a missing context.
Proposed changes
Problem
FILESYSTEM_Mdispatches IO ops from a bthread to anAsyncIOworker pthread that is not bound to any task and therefore has noThreadContext. When the dispatchedfn()reads a local file, it goes throughLocalFileReader::read_at_impl->LIMIT_LOCAL_SCAN_IO->thread_context(), which raisesStatus::FatalError(aborting the BE in debug/ASAN builds) when noThreadContextexists.Observed as a Cloud P0 coredump on the load error-log S3 upload path:
Fix
Initialize an (unattached)
ThreadContextat theAsyncIOworker boundary viaSCOPED_INIT_THREAD_CONTEXT(), the same pattern used by StorageEngine background threads. This gives everythread_context()consumer running on an AsyncIO worker (memory tracking,LIMIT_*_SCAN_IO, ...) a valid context, fixing the crash at its root rather than making individual consumers tolerate a missing context.