Skip to content

Commit 936df2b

Browse files
morningmanclaude
andcommitted
[fix](profile) Do not take the Profile monitor when rendering a profile
### What problem does this PR solve? Issue Number: None Related PR: #66227 Problem Summary: The previous commit made the two completion-state helpers of `Profile` synchronized on the Profile monitor, as branch-4.1 #66227 does. That inverts the lock order with `ProfileManager`: `ProfileManager.getProfile()` renders a profile under the manager's read lock, and `getProfileByLevel()` then waits for the Profile monitor, while `Profile.updateSummary()` holds the monitor and calls `ProfileManager.pushProfile()`, which waits for the manager's write lock (`clearExecutionProfiles()` and `removeProfile()` do the same). Viewing a running profile at the moment its query finishes could therefore hang the query and every later profile operation. The helpers now synchronize on a dedicated lock instead. Under it they only take the execution profile's report lock and the summary's info-string lock, and neither of those is held while waiting for the Profile monitor or a manager lock, so the storage and display updates stay serialized without a cycle. ### Release note None ### Check List (For Author) - Test: Unit Test - `ProfileCompletionStateTest#renderingDoesNotWaitForProfileMonitor` renders a profile while the test holds the Profile monitor; with the synchronized helpers it times out. `ProfileCompletionStateTest` and `ProfileManagerTest` pass (36 tests). - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
1 parent 9c248f8 commit 936df2b

2 files changed

Lines changed: 37 additions & 7 deletions

File tree

‎fe/fe-core/src/main/java/org/apache/doris/common/profile/Profile.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,10 @@ public class Profile {
119119
private String profileStoragePath = "";
120120
private volatile String storageProfileCompletionState;
121121
private volatile boolean executionProfilesReleased;
122+
// Serializes the storage and display updates of the completion state. Not the Profile monitor:
123+
// ProfileManager renders under its read lock, while updateSummary() holds the monitor and then
124+
// takes the manager's write lock.
125+
private final Object completionStateLock = new Object();
122126
// isQueryFinished means the coordinator or stmt executor is finished.
123127
// does not mean the profile report has finished, since the report is async.
124128
// finish of collection of profile is marked by isCompleted of ExecutionProfiles.
@@ -681,15 +685,19 @@ private String getProfileCompletionStateForStorage() {
681685
return SummaryProfile.PROFILE_COMPLETION_STATE_COMPLETE;
682686
}
683687

684-
private synchronized void updateProfileCompletionStateForStorage() {
685-
// Freeze the terminal state before serialization. Concurrent rendering must not
686-
// replace INCOMPLETE with COLLECTING while the storage path is still unpublished.
687-
storageProfileCompletionState = getProfileCompletionStateForStorage();
688-
summaryProfile.setProfileCompletionState(storageProfileCompletionState);
688+
private void updateProfileCompletionStateForStorage() {
689+
synchronized (completionStateLock) {
690+
// Freeze the terminal state before serialization. Concurrent rendering must not
691+
// replace INCOMPLETE with COLLECTING while the storage path is still unpublished.
692+
storageProfileCompletionState = getProfileCompletionStateForStorage();
693+
summaryProfile.setProfileCompletionState(storageProfileCompletionState);
694+
}
689695
}
690696

691-
private synchronized void updateProfileCompletionStateForDisplay() {
692-
summaryProfile.setProfileCompletionState(getProfileCompletionState());
697+
private void updateProfileCompletionStateForDisplay() {
698+
synchronized (completionStateLock) {
699+
summaryProfile.setProfileCompletionState(getProfileCompletionState());
700+
}
693701
}
694702

695703
// Profile IO threads races with Coordinator threads.

‎fe/fe-core/src/test/java/org/apache/doris/common/profile/ProfileCompletionStateTest.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,28 @@ void renderingDuringStoragePreservesTerminalState() throws Exception {
198198
}
199199
}
200200

201+
@Test
202+
void renderingDoesNotWaitForProfileMonitor() throws Exception {
203+
Profile profile = profile();
204+
ExecutionProfile execution = profile.getExecutionProfiles().get(0);
205+
execution.addFragmentBackend(new PlanFragmentId(0), 1L);
206+
profile.markQueryFinished();
207+
report(execution, "127.0.0.1", true);
208+
ExecutorService renderer = Executors.newSingleThreadExecutor();
209+
try {
210+
// updateSummary() holds this monitor while it takes ProfileManager's write lock, and
211+
// ProfileManager renders under its read lock, so rendering must not need the monitor.
212+
synchronized (profile) {
213+
Future<String> rendered = renderer.submit(profile::getProfileByLevel);
214+
Assertions.assertTrue(rendered.get(10, TimeUnit.SECONDS)
215+
.contains("Profile Completion State: COMPLETE"));
216+
}
217+
} finally {
218+
renderer.shutdownNow();
219+
Assertions.assertTrue(renderer.awaitTermination(10, TimeUnit.SECONDS));
220+
}
221+
}
222+
201223
@Test
202224
void persistsTerminalStateBeforeReleasingReports() throws Exception {
203225
for (boolean complete : new boolean[] {false, true}) {

0 commit comments

Comments
 (0)