[delegates] Make the "default" delegates work as expected when they are configured via Eclipse preference

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 403627 | | Status | CLOSED FIXED | | Importance | P3 major | | Reported | Mar 18, 2013 09:22 EDT | | Modified | May 27, 2014 09:52 EDT | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

Coming from Bug 403567

Before Kepler M6 the "default" [1] delegates, for compatibility reasons, worked as they were the classic impl [2] rather than the pivot one[3]. After M6, it works depending on the preferences [4].

For the following example:

package test : test = 'http://test'\ {\ class Foo\ {\ invariant test: Set{1, 1.0}->size() = 1;\ }\ }

Depending of the preference selection.

1. Before Kepler M6, we have the following results \ a) using [1] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\ b) using [2] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\ c) using [3] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\

2. After Kepler M6, we have the following results:\ a) using [1] -> shows exception (No 'http://www.eclipse.org/emf/2002/Ecore/OCL/LPG' ValidationDelegate for 'Foo') when validating Foo instance.\ b) using [2] -> shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\ c) using [3] -> shows exception (No 'http://www.eclipse.org/emf/2002/Ecore/OCL/LPG' ValidationDelegate for 'Foo') when validating Foo instance.

3. For Kepler M7, we need the following results:\ a) using [1] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\ b) using [2] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\ c) using [3] -> Validation is OK when validation Foo instance.\ \ N.B:\ As soon as the "default" delegates are the pivot ones:\ a) using [3] -> Validation is OK when validation Foo instance.\ b) using [2] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance\ c) using [3] -> Validation is OK when validation Foo instance.

[1] http://www.eclipse.org/emf/2002/Ecore/OCL\ [2] http://www.eclipse.org/emf/2002/Ecore/OCL/LPG\ [3] http://www.eclipse.org/emf/2002/Ecore/OCL/Pivot\ [4] Window -> Preference -> OCL -> Executor targeted by default OCL delegate

[eclipse-ocl-bot]

By Ed Willink on Mar 23, 2013 04:31

Bug 403606 has been marked as a duplicate of this bug.

[eclipse-ocl-bot]

By Ed Willink on Mar 23, 2013 11:40

I'm not entirely sure about the repro, since it is unclear which delegate is in use.

The following is unambiguous:

<?xml version="1.0" encoding="UTF-8"?>\ <ecore:EPackage xmi:version="2.0" xmlns:xmi="http://www.omg.org/XMI" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"\ xmlns:ecore="http://www.eclipse.org/emf/2002/Ecore" name="test" nsURI="http://test" nsPrefix="test">\

\

\

\

\ \ \ \

\ \ \

\

\ \ \ so we expect to see exactly one of IsLPG or IsPivot failing. --- The problem was that the Eclipse preference was read and converted to a value-specific derived EValidator.ValidationDelegate class. When the Eclipse setting is changed the old LPG/Pivot class was still used to serve Pivot/LPG. To try to accommodate this there was a WorkaroundValidationDelegate for LPG to Pivot but not vice-versa. This workaround didn't work correctly and so there were Pivot/LPG CCEs. Rather than fix the workaround, it is much easier to fix the real problem of the cached EValidator.ValidationDelegate class. Once the cache is reset by a preference change, the setting can be changed, while the Reflective Ecore Editor is still open, causing the cache to be reset. The next validation then recreates the cache with the new value. The 'wrong' DelegateEClassifierAdapter etc remain but are not used, unless the setting is reverted, so an EClassifier may find itself with both LPG and Pivot DelegateEClassifierAdapters; no problem and not a major memory cost and it only occurs for those changing the setting. --- Next problem. The virtual delegate can be selected for delegation leading to an infinite re-delegation loop. The virtual delegate is no longer one of the options. --- Fix(es) pushed to edw/403627. Please review for M7. Further standalone usage fix is on edw/403595.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Apr 04, 2013 12:38

Ok.

I've done some successfully tests using branch edw/403595 which includes the fixes for this bug.

1. Setting "default" delegates [1] (as the bugzilla title and description explains -> therefore, omitted in the repro) in the metamodel annotations, changing the available preferences (now we have two):\ a) using classic delagates [2] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance.\ b) using pivot delagates [3] -> Validation is OK when validation Foo instance.\

2. Setting classic delegates [2] in the metamodel annotations, \ a) using classic delagates [2] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance.\ b) using pivot delagates [3] -> Shows a validation error (using the 'test ' constraint is violated on 'Foo') when validating Foo instance.

3. Setting pivot delegates [3] in the metamodel annotations:\ a) using classic delagates [2] -> Validation is OK when validation Foo instance.\ b) using pivot delagates [3] -> Validation is OK when validation Foo instance.

So, I have now the expected results.

Your comments to solve the bug also make sense to me.

Code review comments:

Preference.java class

• Can you clarify the point of the synchronized methods here. Will we have different threads trying to listen the same Eclipse preferences ?. I don't like partial solutions to thread-safety, just a source of confusion to the reviewer and further maintainer.

OCLValidationDelegateMapping.java class

• When I see some addListener call, and I don't see the corresponding removeListener counterpart, this sounds to me like a memory leakage source.

ValidationBehaviour.java class (Pivot Version)

• These changes look like debugging-related changes. Do we want them in the repo ?
• Similar comment to the classic version one.

[1] http://www.eclipse.org/emf/2002/Ecore/OCL\ [2] http://www.eclipse.org/emf/2002/Ecore/OCL/LPG\ [3] http://www.eclipse.org/emf/2002/Ecore/OCL/Pivot

[eclipse-ocl-bot]

By Ed Willink on Apr 05, 2013 01:48

(In reply to comment #3)

Preference.java class

• Can you clarify the point of the synchronized methods here. Will we have different threads trying to listen the same Eclipse preferences ?. I don't like partial solutions to thread-safety, just a source of confusion to the reviewer and further maintainer.

For the Pivot code, I'm tentatively claiming thread safety. Difficult to go further without some compile-time checking approach.

Concurrency between main (editor) thread, validation thread and builder thread is a major hazard. All 'static' context must be guarded.

OCLValidationDelegateMapping.java class

• When I see some addListener call, and I don't see the corresponding removeListener counterpart, this sounds to me like a memory leakage source.

The only place to put removeListener would be in some shutdown method that has no obvious requirement. There is only one OCLValidationDelegateMapping so once you warm up OCL it stays until you kill the VM.

ValidationBehaviour.java class (Pivot Version)

• These changes look like debugging-related changes. Do we want them in the repo ?
• Similar comment to the classic version one.

They do no harm (execution speed or code size). They give better stack traces if NPEs occur. They are helpful if this ever needs debugging again, so I'm inclined to leave them.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Apr 05, 2013 05:48

(In reply to comment #4)

(In reply to comment #3)

Preference.java class

• Can you clarify the point of the synchronized methods here. Will we have different threads trying to listen the same Eclipse preferences ?. I don't like partial solutions to thread-safety, just a source of confusion to the reviewer and further maintainer.

For the Pivot code, I'm tentatively claiming thread safety. Difficult to go further without some compile-time checking approach.

Just some clever test cases could try to verify that...

Concurrency between main (editor) thread, validation thread and builder thread is a major hazard. All 'static' context must be guarded.

Yes, I only use synchronization when dealing with modifiable class variables (static) which are shared by multiple instances of the same class, and this is not the case, then my doubts. If you think that the same preference object may be accessed by different threads, why not synchronizing the fireChange method which accesses the listeners array, why not synchronizing the access to defaultValue variable which may be read and write by different threads ?. As I commented I don't like partial solutions, which as you say we can't easily verify right now, and just makes the reviewer and further maintainer thinking about if the code is really thread-safe or it's not, and I can assert that just viewing the code of one class (this one I'm reviewing).

If you want thread-safety of the code, I'd firstly start trying to build some test cases which demonstrate that it's not, then fix everything as appropriate. I'm not sure if this is currently a high priority...

On the other hand, if you still think that this class undoubtedly needs any thread-safety mechanism, have a look to my comments above.\

OCLValidationDelegateMapping.java class

• When I see some addListener call, and I don't see the corresponding removeListener counterpart, this sounds to me like a memory leakage source.

The only place to put removeListener would be in some shutdown method that has no obvious requirement. There is only one OCLValidationDelegateMapping so once you warm up OCL it stays until you kill the VM.

I trust you ;)

ValidationBehaviour.java class (Pivot Version)

• These changes look like debugging-related changes. Do we want them in the repo ?
• Similar comment to the classic version one. They do no harm (execution speed or code size). They give better stack traces if NPEs occur. They are helpful if this ever needs debugging again, so I'm inclined to leave them.

Ok, as you prefer.

To have my +1, you need either remove the synchronization footprints, or include the suggestions I did above.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Apr 05, 2013 07:18

(In reply to comment #5)\ What is thread safety?

I'm primarily concerned with write-versus-write conflict, since on a good day read-versus-write just gives the reader an indeterminacy on the before/after result.

fireChanged certainly has an opportunity for a nasty read-versus-write conflict, if the changed calls back to remove itself, or something else from the list. Possibly a CME.

synchronizing fireChanged is undesirable since you then introduce excellent opportunities for deadlock if changed() is non-trivial. I like my synchronized code to have a good prospect of compelting quickly and locally.

cloning the listeners before iterating can avoid the CME at the expense of a list copy for every fireChanged.

What does the res of Eclipse do?

SWT: EventTable.sendEvent could do really nasty things if a listener was removed. So SWT has this convention that you must do things on the UI thread.

EMF: EModelElementImpl.getEAnnotation will do very nasty things if an annotation is removed or the backing data is resized.

EMF claims to be threadsafe. Perhaps it is, but only if its client code observes sensible undocumented practices.

So:

getDefaultValue/setDefaultValue needs no synchronization becuase it's a simple before/after. synchronization would slow things down but make no difference to a conflict.

fireChanged could be synchronized with limited benefit and some additional hazards. It complies with the reasonable expectation that callbacks should be well-behaved and lightweight. I see no comments on EMF notifier or SWT listener classes of this expectation.

---

Suggesting writing concurrency test cases is not constructive.

A blunderbuss Monte-Carlo test that thrashes concurrency in the hope of seeing a failure should have a low (zero) yield and will probably fail to diagnose any problem that it finds. The failure won't be reproducible.

A targetted test for a particular mechanism will take ten times longer to write and debug than reviewing the target method. My suspicion is that there are over a thousand methods that might be vulnerable.

If it was easy to write a test I would have long ago.

A much more promising approach is to provide password objects so that concurrency is correct by design and compiler verification. Not a topic for this Bugzilla.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Apr 05, 2013 08:50

(In reply to comment #6)

(In reply to comment #5) What is thread safety?

That's a good point... which looks like to start a new long discussion (/sigh) ... but I've gained that myself :P.

I could start looking for and reading tons of topics about thread-safety, but I prefer not to spend more time on that right now. I prefer to reply you with my current knowledge and just learn from your corrections.

I'm primarily concerned with write-versus-write conflict, since on a good day read-versus-write just gives the reader an indeterminacy on the before/after result.

So, I guess you are focusing on partial solution to the thread-safety... or which is your definition of "thread-safety" ?

fireChanged certainly has an opportunity for a nasty read-versus-write conflict, if the changed calls back to remove itself, or something else from the list. Possibly a CME.

synchronizing fireChanged is undesirable since you then introduce excellent opportunities for deadlock if changed() is non-trivial. I like my synchronized code to have a good prospect of compelting quickly and locally.

Here we need the definition of what is thread-safety. To me, once listeners object is acquired, no other thread should be allowed to access the listeners for a particular preference object until the fireChanged finishes (well, until listeners object acquirement is released). I agree that deadlocks may occur if you are not careful enough... but in the end, I'm not sure what you trying to solve from the point of view of thread-safety (well, from my POV of it).

cloning the listeners before iterating can avoid the CME at the expense of a list copy for every fireChanged.

So we agree that something has to be done here. Just an agreement of what is thread-safety is required, then ...

What does the res of Eclipse do?

SWT: EventTable.sendEvent could do really nasty things if a listener was removed. So SWT has this convention that you must do things on the UI thread.

Just one thread supposed... good for SWT, no headaches for them here.

EMF: EModelElementImpl.getEAnnotation will do very nasty things if an annotation is removed or the backing data is resized.

EMF claims to be threadsafe. Perhaps it is, but only if its client code observes sensible undocumented practices.

I have enough with your claims of having a single thread-safe class.

So:

getDefaultValue/setDefaultValue needs no synchronization becuase it's a simple before/after. synchronization would slow things down but make no difference to a conflict.

Let's firstly define what is thread-safety (I've definitely added a TODO to read about it again... /sigh). It looks like that you only want to avoid CME...

fireChanged could be synchronized with limited benefit and some additional hazards. It complies with the reasonable expectation that callbacks should be well-behaved and lightweight. I see no comments on EMF notifier or SWT listener classes of this expectation.

---

Suggesting writing concurrency test cases is not constructive.

A blunderbuss Monte-Carlo test that thrashes concurrency in the hope of seeing a failure should have a low (zero) yield and will probably fail to diagnose any problem that it finds. The failure won't be reproducible.

A targetted test for a particular mechanism will take ten times longer to write and debug than reviewing the target method. My suspicion is that there are over a thousand methods that might be vulnerable.

If it was easy to write a test I would have long ago.

I think it should not be difficult writing a test to demonstrate that a set of classes are not thread-safe. Of course it's not in my priority queue, but sometimes my curiosity disrupts any prioritized queue.

A much more promising approach is to provide password objects so that concurrency is correct by design and compiler verification. Not a topic for this Bugzilla.

It's much easier not claiming what you can assure, but I don't think I'll be able no change your claiming :). Like in science, I'd need facts/proofs to take a theory down.... without a test case which demonstrates your pretentiously false claims I can't go further ;).

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Apr 05, 2013 08:54

BTW, to move forward... having properly implemented thread-safe preferences, is beyond this particular bugzilla which doesn't deserve be delayed. I'm looking forward to reading your reply, though ;)

So +1 to the patch (regardless you include synchronization mechanisms, or you don't).

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Apr 05, 2013 10:06

Pushed to master for M7.

[eclipse-ocl-bot]

By Ed Willink on May 27, 2014 09:43

CLOSED after more than a year in RESOLVED state.

[eclipse-ocl-bot]

By Ed Willink on May 27, 2014 09:52

and CLOSE