AbstractDocument fix concurrency issues (BadPositionCategoryException)

[jukzi]

By synchronized access to members used from different thread

eclipse-jdt/eclipse.jdt.ui#1067

[jukzi]

i wonder if adding "synchronized" to the method is an API change as there is an error if overwriting methods do not add "synchronized" too

[iloveeclipse]

i wonder if adding "synchronized" to the method is an API change as there is an error if overwriting methods do not add "synchronized" too

There is SynchronizableDocument. I believe AbstractDocument was supposed not to be synchronized

[laeubi]

It looks wrong to synchronize on the document that then potentially uses a lock object.
If synchronization is required, the lock object is needed to be set.

Also see comment here I think it is better solved on the consumer side:

• eclipse-jdt/eclipse.jdt.ui#1067 (comment)

[eclipse-platform-bot]

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.core.filebuffers/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 91fb4bd575603c8924b4430bf76c060c47c48e50 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Wed, 8 Jan 2025 14:22:19 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/bundles/org.eclipse.core.filebuffers/META-INF/MANIFEST.MF b/bundles/org.eclipse.core.filebuffers/META-INF/MANIFEST.MF
index 2049ed7c8a..4e74423384 100644
--- a/bundles/org.eclipse.core.filebuffers/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.core.filebuffers/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.core.filebuffers; singleton:=true
-Bundle-Version: 3.8.300.qualifier
+Bundle-Version: 3.8.400.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Export-Package: 
-- 
2.47.1

Further information are available in Common Build Issues - Missing version increments.

[jukzi]

What do you suggest?

Checking all usages seem to be impossible by the shear amount of usages.
One problem might be that SynchronizableDocument does not even offer/override addPositionCategory which will be the root cause to unsynchronized update the object. Another problem is that using the SynchronizableDocument will never reveal the stacktrace/thread which illegally called addPositionCategory in another thread.

[github-actions]

Test Results

 1 818 files  ±0   1 818 suites  ±0   1h 34m 22s ⏱️ - 4m 20s
 7 732 tests ±0   7 503 ✅  - 1  228 💤 ±0  1 ❌ +1 
24 357 runs  ±0  23 607 ✅  - 1  749 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 4660e33. ± Comparison against base commit e3382fc.

♻️ This comment has been updated with latest results.

[iloveeclipse]

i wonder if adding "synchronized" to the method is an API change as there is an error if overwriting methods do not add "synchronized" too

See https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/Evolving-Java-based-APIs-2.md

Adding or removing the synchronized modifier also has a bearing on the method's behavior in a multi-threaded world, and may therefore raise a question of contract compatibility.

@jukzi : I'm not sure it is a good idea to change behavior of the basic class for documents with multiple inheritance, I see at least two clients that had extra synchronization enabled - SynchronizableDocument and XtextDocument.

@szarnekow : FYI. XtextDocument uses own locking too.

[jukzi]

I changed the commit to only synchronize SynchronizableDocument where needed/missing

[laeubi]

Looks fine to me

[iloveeclipse]

Thanks, looks not so scary anymore.

[jukzi]

ignoring random fail #1517

[szarnekow]

@szarnekow : FYI. XtextDocument uses own locking too.

Thanks for the ping. I was already reading the MR and it looks quite good. Nice catch @jukzi