Make sure Launch Bar Manager sends event at a correct time by DangMinhTam382 · Pull Request #1486 · eclipse-cdt/cdt

DangMinhTam382 · 2026-06-25T09:18:05Z

When active descriptor changed, and fireActiveLaunchDescriptorChanged() is called, current active mode and target are out of sync. This puts listeners at risk of handling event with false data for the changed descriptor.

Changed it so that only fire event when related fields are finished updating

github-actions · 2026-06-25T11:06:13Z

Test Results

0 files - 216 0 suites - 216 0s ⏱️ - 25m 8s
0 tests - 5 937 0 ✅ - 5 806 0 💤 - 131 0 ❌ ±0
0 runs - 5 936 0 ✅ - 5 805 0 💤 - 131 0 ❌ ±0

Results for commit 2965707. ± Comparison against base commit 8a79681.

♻️ This comment has been updated with latest results.

DangMinhTam382 · 2026-06-25T11:19:16Z

Hi Mr. @betamaxbandit ,
CC: Mr. @Kummallinen ,

Before making this PR official, could you take a quick look and let me know what you think?

Thanks!

DangMinhTam382

Hi Mr. @betamaxbandit ,
CC: Mr. @Kummallinen ,

I added some UTs and replied to your comments on this PR.
Please have a look at it again.

Also, build is failed atm with errors:

[ERROR] org.eclipse.linuxtools.docker.editor.ls 1.0.1.202609160755 requires Execution Environment that matches (&(osgi.ee=JavaSE)(version=25)) but the current resolution context uses [a.jre.javase 21.0.0]
[ERROR] Cannot resolve target definition:
[ERROR] Software being installed: org.eclipse.linuxtools.docker.feature.feature.group 5.25.0.202609160755
[ERROR] Missing requirement for filter properties ~= $0: org.eclipse.linuxtools.docker.editor.ls 1.0.1.202609160755 requires 'osgi.ee; (&(osgi.ee=JavaSE)(version=25)), filter=(!(org.eclipse.equinox.p2.disable.require.capability.osgi.ee=true))' but it could not be found
[ERROR] Cannot satisfy dependency: org.eclipse.linuxtools.docker.editor.ls.feature.feature.group 5.25.0.202609160755 depends on: org.eclipse.equinox.p2.iu; org.eclipse.linuxtools.docker.editor.ls [1.0.1.202609160755,1.0.1.202609160755]
[ERROR] Cannot satisfy dependency: org.eclipse.linuxtools.docker.feature.feature.group 5.25.0.202609160755 depends on: org.eclipse.equinox.p2.iu; org.eclipse.linuxtools.docker.editor.ls.feature.feature.group [5.25.0.202609160755,5.25.0.202609160755]
[ERROR]

Maybe something not working on the server?
Will look into this.

betamaxbandit · 2026-09-28T16:52:23Z

+	 * @throws InterruptedException
+	 */
+	@Test
+	public void launchBarStateTest_LaunchDescriptorChanged() throws CoreException, InterruptedException {


Hi @DangMinhTam382 ,

thanks for adding the regression test. I think this is good since you added launchBarStateTest_LaunchDescriptorChanged; it's a good test.

But not sure about launchBarStateTest_LaunchTargetChanged and launchBarStateTest_LaunchModeChanged. Do they fail without the current fix in? I might have misread this so check what I'm saying please.

Even though I think I guided you against doing this I think it might be better to test all three states in the single test.

Set the exact state the descriptor test currently prepares:
before switch:
descriptor2 / target2 / run

switch to:
descriptor1 / target1 / preferredMode1

Then install one listener implementing all three callbacks:
activeLaunchDescriptorChanged(...)
activeLaunchTargetChanged(...)
activeLaunchModeChanged(...)

For every callback generated by
setActiveLaunchDescriptor(desc_no1)

capture:
getActiveLaunchDescriptor()
getActiveLaunchTarget()
getActiveLaunchMode()

and afterwards verify that every captured state was:
descriptor1 / target1 / preferredMode1

That would directly test the stated design:
"Whenever an event is fired, launch bar manager is expected to have up-to-date fields."

Hi Mr. @betamaxbandit ,

But not sure about launchBarStateTest_LaunchTargetChanged and launchBarStateTest_LaunchModeChanged. Do they fail without the current fix in? I might have misread this so check what I'm saying please.

It does not fail without fix.
I added them as an extra cover for the ILaunchBarListener.class
3 UT I added is testing for 3 checkpoint:

Launch descriptor changed

Launch target changed

Launch mode changed

And after the change happens, launch bar fields is capture each time an event is catched in ILaunchBarListener.class.

That would directly test the stated design:
"Whenever an event is fired, launch bar manager is expected to have up-to-date fields."

That's a good idea.
I updated the UT.
Now, the test ILaunchBarListener captures all events and records the launch bar state at that time.
All those records are verified afterward.

betamaxbandit

This is looking much better now the regression has been added. I have added a few comments about the extra tests.
See what you think Tam.

DangMinhTam382

Hi Mr. @betamaxbandit ,
Thanks for the comments.
The suggestion you made on the UT is really on point.
I updated them and this should be ready now

DangMinhTam382 · 2026-09-30T10:26:53Z

+	 * @throws InterruptedException
+	 */
+	@Test
+	public void launchBarStateTest_LaunchDescriptorChanged() throws CoreException, InterruptedException {


Hi Mr. @betamaxbandit ,

But not sure about launchBarStateTest_LaunchTargetChanged and launchBarStateTest_LaunchModeChanged. Do they fail without the current fix in? I might have misread this so check what I'm saying please.

It does not fail without fix.
I added them as an extra cover for the ILaunchBarListener.class
3 UT I added is testing for 3 checkpoint:

Launch descriptor changed

Launch target changed

Launch mode changed

And after the change happens, launch bar fields is capture each time an event is catched in ILaunchBarListener.class.

That would directly test the stated design:
"Whenever an event is fired, launch bar manager is expected to have up-to-date fields."

That's a good idea.
I updated the UT.
Now, the test ILaunchBarListener captures all events and records the launch bar state at that time.
All those records are verified afterward.

betamaxbandit

Tam, this is looking good. I think it could be merged now.
I only have one slight improvement for the test if you feel it would add value.

betamaxbandit · 2026-10-12T18:18:47Z

+				state.put(attr_activeDesc, descriptor);
+				state.put(attr_activeTarget, launchBarManagerMock.getActiveLaunchTarget());
+				state.put(attr_activeMode, launchBarManagerMock.getActiveLaunchMode());
+				launchBarState.get().add(state);


This now covers the state seen by all callbacks during the descriptor transition, which looks good.
One concern: the recorded state does not identify which callback produced it. Therefore launchBarStateTest_LaunchDescriptorChanged() could still pass if activeLaunchDescriptorChanged() were never fired, provided another callback populated the list. An empty list would also pass the assertions vacuously.
Could we record the event type as well and assert that the expected event was actually received? I don't think we need to assert event ordering, just that the expected callback occurred and that every callback saw the correct Launch Bar state.

I think that a good point, especially

An empty list would also pass the assertions vacuously.

Had some thoughts on this one, and decided the create a TestLaunchBarListener object to make the event handling record more efficient.

Each event type will be record correspondingly and verified separately.

DangMinhTam382

Hi Mr. @betamaxbandit ,
Thanks for the comments.
I updated the UT to cover for Event's type verification.

Could you have a look at it again and let me know what you think?

Thanks!

DangMinhTam382 · 2026-10-13T07:58:10Z

+				state.put(attr_activeDesc, descriptor);
+				state.put(attr_activeTarget, launchBarManagerMock.getActiveLaunchTarget());
+				state.put(attr_activeMode, launchBarManagerMock.getActiveLaunchMode());
+				launchBarState.get().add(state);


I think that a good point, especially

An empty list would also pass the assertions vacuously.

Had some thoughts on this one, and decided the create a TestLaunchBarListener object to make the event handling record more efficient.

Each event type will be record correspondingly and verified separately.

betamaxbandit

Hi @Tam,

your latest change looks very good - thank you.

I'm sorry, but I only just noticed that your new test file does not have a copyright header.

Once you've added that, I think the change is ready to merge.

When active descriptor changed, and fireActiveLaunchDescriptorChanged() is called, current active mode and target are out of sync. This puts listeners at risk of handling event with false data for the changed descriptor. Changed it so that only fire event when related fields are finished updating

DangMinhTam382

Hi Mr. @betamaxbandit ,

Thanks for pointing this out.
I seem to keep forgetting this one.
The copyright header has been added.

Also, made this PR official as well.

Many thanks!

betamaxbandit

Hi @DangMinhTam382
LGTM.
Thanks for fixing the copyright. I have no further comments.
However, I'd like @Kummallinen to give a final review because he did have a previous comment.

Also you mentioned there is a test failure and I understand that you are currently attempting to identify whether this is related to your change.

requested changes have no been made.

DangMinhTam382 · 2026-10-14T12:12:51Z

Hi Mr. @betamaxbandit ,
CC: @Kummallinen ,

Also you mentioned there is a test failure and I understand that you are currently attempting to identify whether this is related to your change.

That's correct.
Looking at the log, the test seems to be cancelled after a long timeout when running CBuildConfigurationManagerTests

2026-10-14T03:07:18.3083423Z Running org.eclipse.cdt.core.build.CBuildConfigurationManagerTests
2026-10-14T03:07:18.3702636Z Mockito is currently self-attaching to enable the inline-mock-maker. This will no longer work in future releases of the JDK. Please add Mockito as an agent to your build as described in Mockito's documentation: https://javadoc.io/doc/org.mockito/mockito-core/latest/org.mockito/org/mockito/Mockito.html#0.3
2026-10-14T03:07:18.5849449Z WARNING: A Java agent has been loaded dynamically (/home/runner/.m2/repository/p2/osgi/bundle/net.bytebuddy.byte-buddy-agent/1.18.15/net.bytebuddy.byte-buddy-agent-1.18.15.jar)
2026-10-14T03:07:18.5850399Z WARNING: If a serviceability tool is in use, please run with -XX:+EnableDynamicAgentLoading to hide this warning
2026-10-14T03:07:18.5851431Z WARNING: If a serviceability tool is not in use, please run with -Djdk.instrument.traceUsage for more information
2026-10-14T03:07:18.5852019Z WARNING: Dynamic loading of agents will be disallowed by default in a future release
2026-10-14T03:07:18.5944392Z OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
2026-10-14T05:03:13.8922975Z ##[error]The operation was canceled.
2026-10-14T05:03:13.8991651Z Post job cleanup.

So, I tried running it in my local dev environment and notice that a deadlock situation occurred at random rate (It passed sometime - looks like a racing condition case).

And apparently, with this PR changed, the deadlock happened more frequently.

Not entirely sure, the change's making the issue worst or more noticeable.
I will do more investigation on this matter.

betamaxbandit · 2026-10-15T09:06:54Z

Hi Mr. @betamaxbandit , CC: @Kummallinen ,

Also you mentioned there is a test failure and I understand that you are currently attempting to identify whether this is related to your change.

That's correct. Looking at the log, the test seems to be cancelled after a long timeout when running CBuildConfigurationManagerTests

2026-10-14T03:07:18.3083423Z Running org.eclipse.cdt.core.build.CBuildConfigurationManagerTests
2026-10-14T03:07:18.3702636Z Mockito is currently self-attaching to enable the inline-mock-maker. This will no longer work in future releases of the JDK. Please add Mockito as an agent to your build as described in Mockito's documentation: https://javadoc.io/doc/org.mockito/mockito-core/latest/org.mockito/org/mockito/Mockito.html#0.3
2026-10-14T03:07:18.5849449Z WARNING: A Java agent has been loaded dynamically (/home/runner/.m2/repository/p2/osgi/bundle/net.bytebuddy.byte-buddy-agent/1.18.15/net.bytebuddy.byte-buddy-agent-1.18.15.jar)
2026-10-14T03:07:18.5850399Z WARNING: If a serviceability tool is in use, please run with -XX:+EnableDynamicAgentLoading to hide this warning
2026-10-14T03:07:18.5851431Z WARNING: If a serviceability tool is not in use, please run with -Djdk.instrument.traceUsage for more information
2026-10-14T03:07:18.5852019Z WARNING: Dynamic loading of agents will be disallowed by default in a future release
2026-10-14T03:07:18.5944392Z OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
2026-10-14T05:03:13.8922975Z ##[error]The operation was canceled.
2026-10-14T05:03:13.8991651Z Post job cleanup.

So, I tried running it in my local dev environment and notice that a deadlock situation occurred at random rate (It passed sometime - looks like a racing condition case). And apparently, with this PR changed, the deadlock happened more frequently.

Not entirely sure, the change's making the issue worst or more noticeable. I will do more investigation on this matter.

Hi @DangMinhTam382 ,

I had a quick look at this issue and tests around it. I didn't come up with any silver bullets, I'm wondering if what this test is doing could be a clue or help?

cdt/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/build/CBuildConfigurationManagerTests.java

Line 212 in 8a79681

waitForLaunchBarTracker();

DangMinhTam382 · 2026-10-16T12:19:22Z

Hi Mr. @betamaxbandit,
CC: Mr. @Kummallinen

Thanks for the suggestion on the waitForLaunchBarTracker();.
It does help with the UT and make it stable.

Though, this not going to be the suitable fix, as it happens in multiple Test class. Fixing for each of them is not an option here.
So, I've investigated it and come up with a different approach in #1534

Also, had a theory on how this PR impact the other UT: (#1533 (comment))

The main noticeable difference is that PR 1486 delays the notification sent by LaunchBarManager to CoreBuildLaunchBarTracker. This could potentially reduce the time gap between the two call flows above, making the race condition more likely to occur.

There's more information on the issue.
I mentioned you there, if you have a moment, please help me have a quick look at it.

Many thanks!

Kummallinen reviewed Jun 30, 2026

View reviewed changes

Comment thread ...bar/org.eclipse.launchbar.core/src/org/eclipse/launchbar/core/internal/LaunchBarManager.java Resolved

betamaxbandit previously requested changes Sep 8, 2026

View reviewed changes

DangMinhTam382 force-pushed the LaunchBarNewEvent branch 2 times, most recently from 17292ff to aa7a299 Compare September 16, 2026 12:02

DangMinhTam382 commented Sep 16, 2026

View reviewed changes

betamaxbandit reviewed Sep 28, 2026

View reviewed changes

Comment thread ...lipse.launchbar.core.tests/src/org/eclipse/launchbar/core/internal/LaunchBarManagerTest.java Resolved

betamaxbandit reviewed Sep 28, 2026

View reviewed changes

DangMinhTam382 commented Sep 30, 2026

View reviewed changes

betamaxbandit reviewed Oct 12, 2026

View reviewed changes

DangMinhTam382 commented Oct 13, 2026

View reviewed changes

betamaxbandit reviewed Oct 13, 2026

View reviewed changes

Comment thread ...aunchbar.core.tests/src/org/eclipse/launchbar/core/internal/dummy/TestLaunchBarListener.java Resolved

DangMinhTam382 added 6 commits October 14, 2026 10:01

Bump version

5932ab4

Add 3 UT test cases that can detect original issue

d4622cc

Updated Unit test to make sure it cover all edge case

da2b710

Updated UT, Making sure that expected event types is verified in the UT

9c86e31

Added copy right

8439da4

DangMinhTam382 force-pushed the LaunchBarNewEvent branch from 8c0173b to 8439da4 Compare October 14, 2026 03:02

DangMinhTam382 commented Oct 14, 2026

View reviewed changes

Comment thread ...aunchbar.core.tests/src/org/eclipse/launchbar/core/internal/dummy/TestLaunchBarListener.java Resolved

DangMinhTam382 marked this pull request as ready for review October 14, 2026 04:35

betamaxbandit reviewed Oct 14, 2026

View reviewed changes

betamaxbandit mentioned this pull request Oct 14, 2026

Launch Bar & Core Build (CMake) Improvements #1528

Open

DangMinhTam382 force-pushed the LaunchBarNewEvent branch from 2965707 to 4d8f585 Compare October 16, 2026 06:27

DangMinhTam382 mentioned this pull request Oct 16, 2026

Deadlock between CoreBuildLaunchBarTracker job and CBuildConfigurationManager.createBuildConfiguration() #1533

Open

DangMinhTam382 marked this pull request as draft October 16, 2026 12:13

DangMinhTam382 force-pushed the LaunchBarNewEvent branch from 4d8f585 to 8439da4 Compare October 19, 2026 10:09

betamaxbandit mentioned this pull request Nov 3, 2026

Fix deadlock for CBuildConfigurationManager and CoreBuildLaunchBarTracker #1534

Open

Conversation

DangMinhTam382 commented Jun 25, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

DangMinhTam382 commented Jun 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DangMinhTam382 left a comment

Choose a reason for hiding this comment

Uh oh!

betamaxbandit Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

DangMinhTam382 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

betamaxbandit left a comment

Choose a reason for hiding this comment

Uh oh!

DangMinhTam382 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DangMinhTam382 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

betamaxbandit left a comment

Choose a reason for hiding this comment

Uh oh!

betamaxbandit Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

DangMinhTam382 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

DangMinhTam382 left a comment

Choose a reason for hiding this comment

Uh oh!

DangMinhTam382 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

betamaxbandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DangMinhTam382 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

betamaxbandit left a comment

Choose a reason for hiding this comment

Uh oh!

DangMinhTam382 commented Oct 14, 2026

Uh oh!

betamaxbandit commented Oct 15, 2026

Uh oh!

DangMinhTam382 commented Oct 16, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jun 25, 2026 •

edited

Loading