Doris-Breakwater commented on issue #66311: URL: https://github.com/apache/doris/issues/66311#issuecomment-5138477331
Breakwater-GitHub-Analysis-Slot: slot_c23c27703629 ## Initial triage **Confirmed code-level concurrency/resource-management bug on current `master`** (reviewed at `687500b3358a3bcc01ca095e1371a8d1f9a51fd4`). The issue is currently unlabeled; `area/profile` is an appropriate existing label. The main report is supported by the code: - [`runAfterCatalogReady()`](https://github.com/apache/doris/blob/687500b3358a3bcc01ca095e1371a8d1f9a51fd4/fe/fe-core/src/main/java/org/apache/doris/common/profile/ProfileManager.java#L548-L553) calls `loadProfilesFromStorageIfFirstTime(false)` on every `ProfileManager` cycle; the default cycle interval is one second. - [`loadProfilesFromStorageIfFirstTime`](https://github.com/apache/doris/blob/687500b3358a3bcc01ca095e1371a8d1f9a51fd4/fe/fe-core/src/main/java/org/apache/doris/common/profile/ProfileManager.java#L601-L681) checks only the completed flag and then performs a non-atomic check/start sequence. Until one loader reaches the final `isProfileLoaded = true`, every cycle may start another `profile-loader`. - Both storage enumeration and the untimed `Future.get()` calls can keep a loader alive indefinitely. Concurrent loaders then duplicate reads, consume the shared profile IO pool/queue, and add waiting threads. - `isProfileLoaded` remains false when the outer load operation fails before its success assignment, so the scheduler retries immediately on later cycles. There are two precision points: 1. `Profile.read()` catches ordinary `Exception`s and returns `null`, and each `future.get()` exception is also caught. A malformed/read-failing individual profile therefore normally does **not** take the outer failure path; blocking IO, executor submission rejection, or another exception outside those inner catches does. 2. The fixed IO executor has a bounded queue and `BlockedPolicy(..., 60)`. When the queue is full, a submitter can wait for up to 60 seconds and then terminate with `RejectedExecutionException`. Thus the exact live-thread curve under pure queue saturation is workload-dependent rather than necessarily monotonically unbounded forever. This does not change the defect: the current code can create many overlapping loader threads, and a block before executor submission can grow them without a bound. ## Fix requirements The proposed single-flight direction is correct, but a bare `AtomicBoolean` is insufficient for all stated semantics: - Make the completed-state check and acquisition of the in-flight load atomic, so at most one load can be started. - Keep a shared in-flight completion handle (`Future`, `CompletableFuture`, latch/condition, or equivalent). A `sync=true` caller arriving during an asynchronous load must await that existing load rather than return early or start a second one. - Set `isProfileLoaded` only after the chosen successful-completion condition. Clear the in-flight state and wake synchronous waiters in `finally`. - Define failure retry semantics explicitly. Clearing `isProfileLoading` in `finally` alone permits a fast failure to spawn a new loader on the next one-second cycle. To satisfy the issue's expectation, use a retry backoff/cooldown or a separate terminal attempted/failed state; setting `isProfileLoaded=true` after an unsuccessful load would suppress retries and should only be done deliberately. - Preserve the interrupt status when a synchronous waiter is interrupted. - If recovery from permanently blocked storage IO is required, single-flight bounds the thread leak but does not make the cold load recover. That needs a timeout/cancellation policy or isolation from the shared profile IO executor. ## Tests requested for the PR 1. Block storage enumeration or a read with a latch, invoke the method repeatedly/asynchronously, and assert exactly one cold-load invocation/loader remains in flight. 2. Call `sync=true` while that load is in flight and verify it waits for the same load and does not create another loader. 3. Inject a deterministic outer failure or executor rejection and verify the selected retry/backoff state transition and cleanup of the in-flight state. 4. Verify a successful load still sets `isProfileLoaded` and subsequent scheduler cycles do not start another load. 5. Verify interrupted synchronous waiting restores the thread's interrupt flag. No production logs are required to establish the race from source. To quantify severity on an affected FE, a timestamped series of thread dumps plus the `profile-io-thread-pool` active-thread/queue metrics and the matching FE log interval would be useful. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
