Few WorkManager fields are not MT safe

[iloveeclipse]

Few fields are not volatile/atomic but could be accessed from different threads, so even if the modification happens while holding workspace locks, other threads might see stale values.

Fields in question are

• WorkManager.operationCanceled
• WorkManager.nestedOperations
• WorkManager.preparedOperations

Additionally changed Workspace.endOperation() to read workManager.getPreparedOperationDepth() only once to avoid inconsistent reads.

Fixes https://github.com/eclipse-platform/eclipse.platform/issues/2148

[laeubi]

so even if the modification happens while holding workspace locks, other threads might see stale values

Wo other threads are reading the values outside the workspace lock? Looking at the code changes it seem not to really benefit / improve from the changes otherwise, e.g. the atomic operation is never used to compareAndSet and otherwise only unconditional set values.

[iloveeclipse]

Wo other threads are reading the values outside the workspace lock?

I'm not saying that they are reading outside the lock, I'm saying they are probably reading stale values. WorkManager.operationCanceled() and .WorkManager.shouldBuild() are called from methods that are not synchronized, the filed is not volatile, so IMO other threads might see stale values.

[laeubi]

I'm not saying that they are reading outside the lock, I'm saying they are probably reading stale values.

If they are reading them under the workspace look, how can they ever see stale values?

WorkManager.operationCanceled() and .WorkManager.shouldBuild() are called from methods that are not synchronized, the filed is not volatile, so IMO other threads might see stale values.

I always wondered where the impression comes from that only synchronized methods are barriers and anyone should see "stale" data, if that would be the case we would need synchronized everywhere and java would be completely unusable as a programming language.

[github-actions[bot]]

Test Results

 1 947 files  ±0   1 947 suites  ±0   1h 28m 16s ⏱️ - 9m 58s  4 720 tests ±0   4 696 ✅ ±0   24 💤 ±0  0 ❌ ±0  14 160 runs  ±0  13 993 ✅ ±0  167 💤 ±0  0 ❌ ±0 

Results for commit 647d485b. ± Comparison against base commit 54a6a866.

[iloveeclipse]

I always wondered where the impression comes from that only synchronized methods

I never said that, please carefully quote what I've said.

If they are reading them under the workspace look

The workspace lock I mean is WorkManager.lock, and it is for humans very hard to proof which WorkManager operation is inside a sequence of calls that acquired this lock and how many times the lock was already acquired/released.

Given that, quote myself, note the "might" verb:

Few fields are not volatile/atomic but could be accessed from different threads, so even if the modification happens while holding workspace locks, other threads might see stale values.

Therefore IMO if we know the code is runing in multiple threads, the fields are not volatile, and we can't easily proof all paths that read/write of the fields are properly guarded by either synchronized blocks or the lock above, it is safer to make the code protected from possible MT issues.

[laeubi]

Therefore IMO if we know the code is runing in multiple threads, the fields are not volatile, and we can't easily proof all paths that read/write of the fields are properly guarded by either synchronized blocks or the lock above, it is safer to make the code protected from possible MT issues.

But that's the point, if we can't prove that then the change does not gives us anything better regards MT issues, it would only solve e.g. if we can see that one code hangs forever (e.g. boolean value is not visible due to missing volatile as its cached) but as you said you can't give a proof here.

Second is the update of the filed, now made atomic but still do not prevent from a race condition (that is one Thread is writing a value where another is seeing a stale value and behave wrong).

So I'm really a bit reserved on changing code based on "feelings" ( = might help but might not and we hope it does not harm) instead of at least facts (what would be for example a callstack that shows problematic access or at least a description of the case to fix). You are referencing explicitly an issue in JDT, so how/why would one even expect to make the situation better there with this change?