Fix for multi threading issues in AnnotationMap #3397

mehmet-karaman · 2025-10-13T07:39:02Z

This PR will fix multithreading issues described in Sporadically thrown NPE and NSEE during issue processing after validation. eclipse-xtext/xtext#3524
added synchronize block to methods in AnnotationModel, wherever reads / writes could happen in multiple threads.
changed execution order, to be able to synchronize only necessary code blocks.
Removed dead code in AnnotationModel.cleanup because mapLock can't be null.
Added new javadoc to IAnnotationMap.getLockObject().
added assert.isLegal check to AnnotationMAp.setLockObject()

laeubi · 2025-10-13T07:44:19Z

@mehmet-karaman can you please more explain the rationale behind the changes? I think we can assume code is there for a reason so if we change fundamental things we should carefully describe the reasons and causes and why it is not a bug of the caller for example as otherwise we might run into deadlocks if code that previously run outside locks now run under lock conditions.

iloveeclipse · 2025-10-13T07:45:59Z

The change here is coming from eclipse-xtext/xtext#3524 investigation.

iloveeclipse · 2025-10-13T07:51:32Z

@szarnekow : if you have time, would be good if you could check this PR.

laeubi · 2025-10-13T07:53:42Z

It still would be good to more explain the individual changes and why they are required / needed. Also I think some changes are better a separate PR (as they are easier to review then e.g. "Removed dead code" or API documentation enhancements)

iloveeclipse · 2025-10-13T07:55:39Z

Also I think some changes are better a separate PR

Could you please point which exactly?

laeubi · 2025-10-13T08:03:50Z

Each of those can be an own PR

Removed dead code in AnnotationModel.cleanup because mapLock can't be null.
Added new javadoc to IAnnotationMap.getLockObject()
added assert.isLegal check to AnnotationMAp.setLockObject()

All of these seem independent and local enough to be quickly reviewed and merged and likely will improve things without risk for regression. And even if they are easier to revert in isolation.

Then might be the next thing

changed execution order, to be able to synchronize only necessary code blocks.

and finally

added synchronize block to methods in AnnotationModel, wherever reads / writes could happen in multiple threads.

This will make each PR small, focused and likely better to understand the implications and its easier to write a concise commit message.

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

iloveeclipse · 2025-10-13T08:07:46Z

Each of those can be an own PR

Removed dead code in AnnotationModel.cleanup because mapLock can't be null.

Added new javadoc to IAnnotationMap.getLockObject()

added assert.isLegal check to AnnotationMAp.setLockObject()

I believe these could be seperated from the main PR, but should go in one PR, because they all require each other.

laeubi · 2025-10-13T08:14:37Z

I believe these could be seperated from the main PR, but should go in one PR, because they all require each other.

Sure at laest I think these are more "cleanup" and currently make the PR harder to review than it should.

github-actions · 2025-10-13T08:38:49Z

Test Results

3 018 files ±0 3 018 suites ±0 2h 28m 55s ⏱️ + 7m 28s
8 226 tests ±0 7 976 ✅ - 1 249 💤 ±0 1 ❌ +1
23 598 runs ±0 22 803 ✅ - 1 794 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit 5fa4d84. ± Comparison against base commit 554e227.

♻️ This comment has been updated with latest results.

iloveeclipse · 2025-10-16T15:44:25Z

I've rebased on #3399 state, fixed merge conflicts & my own comment above.

iloveeclipse · 2025-10-17T13:38:13Z

Test failures on Windows are unrelated.

@mehmet-karaman, @laeubi : please feel free to review.

laeubi

It looks ok in general but I have not testes it.

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

mehmet-karaman · 2025-10-20T07:24:26Z

Resolved two remaining comments and rebased on latest master.

iloveeclipse · 2025-10-20T07:32:54Z

@mehmet-karaman : you have now two commits, I assume by mistake. Can you please have one?

- added synchronize block to methods in AnnotationModel, wherever reads / writes could happen in multiple threads. - changed execution order, to be able to synchronize only necessary code blocks. Co-authored-by: Andrey Loskutov <loskutov@gmx.de>

mehmet-karaman · 2025-10-20T07:42:30Z

squashed the two commits.

Copilot

Pull Request Overview

This PR addresses multithreading issues in the AnnotationModel class by adding proper synchronization to prevent race conditions during concurrent access to annotation data. The changes ensure thread safety while maintaining performance by minimizing the scope of synchronized blocks.

Key changes:

Added synchronized blocks around critical sections that access shared data structures
Reorganized execution order to minimize time spent in synchronized blocks
Removed dead code and added validation checks

Copilot · 2025-10-20T08:09:23Z

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

 			if (fireModelChanged && forkNotification) {
 				removeAnnotations(deleted, false, false);
-				synchronized (getLockObject()) {
+				synchronized (mapLock) {


Using 'mapLock' directly instead of 'getLockObject()' is inconsistent with the synchronization pattern used throughout the rest of the class. This should use 'getLockObject()' for consistency and to ensure the same lock object is used.

Suggested change

synchronized (mapLock) {

synchronized (annotations.getLockObject()) {

Copilot · 2025-10-20T08:09:23Z

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java

+		IDocument document= fDocument;
+		if (document != null) {
 			for (int i= 0; i < fOpenConnections; i++) {
 				ret.disconnect(fDocument);


Using 'fDocument' instead of the local 'document' variable creates a potential race condition. The local 'document' variable was captured to avoid accessing the field multiple times, but this line still uses the field directly.

Suggested change

ret.disconnect(fDocument);

ret.disconnect(document);

iloveeclipse reviewed Oct 13, 2025

View reviewed changes

bundles/org.eclipse.text/src/org/eclipse/jface/text/source/AnnotationModel.java Outdated Show resolved Hide resolved

iloveeclipse mentioned this pull request Oct 13, 2025

Code cleanup in AnnotationModel, AnnotationMap, Javadoc inherit adjustment #3399

Merged

iloveeclipse force-pushed the fix_multithreading_problems_in_annotations branch 2 times, most recently from 34fc4f0 to 4082003 Compare October 16, 2025 15:43

iloveeclipse enabled auto-merge (rebase) October 16, 2025 16:36

iloveeclipse disabled auto-merge October 16, 2025 16:39

iloveeclipse force-pushed the fix_multithreading_problems_in_annotations branch 2 times, most recently from f3ef6e9 to e5d2191 Compare October 17, 2025 07:43

laeubi approved these changes Oct 17, 2025

View reviewed changes

mehmet-karaman force-pushed the fix_multithreading_problems_in_annotations branch from e5d2191 to 703a1ad Compare October 20, 2025 07:23

mehmet-karaman force-pushed the fix_multithreading_problems_in_annotations branch from 703a1ad to 5fa4d84 Compare October 20, 2025 07:42

laeubi requested a review from Copilot October 20, 2025 08:08

Copilot AI reviewed Oct 20, 2025

View reviewed changes

	synchronized (mapLock) {
	synchronized (annotations.getLockObject()) {

Fix for multi threading issues in AnnotationMap #3397

Are you sure you want to change the base?

Fix for multi threading issues in AnnotationMap #3397

Conversation

mehmet-karaman commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laeubi commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

laeubi commented Oct 13, 2025

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

laeubi commented Oct 13, 2025

Uh oh!

Uh oh!

iloveeclipse commented Oct 13, 2025

Uh oh!

laeubi commented Oct 13, 2025

Uh oh!

github-actions bot commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

iloveeclipse commented Oct 16, 2025

Uh oh!

iloveeclipse commented Oct 17, 2025

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mehmet-karaman commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iloveeclipse commented Oct 20, 2025

Uh oh!

mehmet-karaman commented Oct 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mehmet-karaman commented Oct 13, 2025 •

edited

Loading

github-actions bot commented Oct 13, 2025 •

edited

Loading

mehmet-karaman commented Oct 20, 2025 •

edited

Loading