Bugzilla – Full Text Bug Listing
| Summary: | Missed notifications in some cases can corrupt the "dirty" state of the session and make it impossible to save changes | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] Sirius | Reporter: | Pierre-Charles David <pierre-charles.david> | ||||||
| Component: | Core | Assignee: | Pierre-Charles David <pierre-charles.david> | ||||||
| Status: | CLOSED FIXED | QA Contact: | Pierre-Charles David <pierre-charles.david> | ||||||
| Severity: | major | ||||||||
| Priority: | P3 | CC: | florian.barbin, laurent.redor | ||||||
| Version: | 4.0.0 | Keywords: | triaged | ||||||
| Target Milestone: | 5.0.0 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=506125 https://bugs.eclipse.org/bugs/show_bug.cgi?id=481433 https://git.eclipse.org/r/94463 https://git.eclipse.org/c/emf-transaction/org.eclipse.emf.transaction.git/commit/?id=03eed78097c7e29b80d91f55a4fbf29b38f8900d |
||||||||
| Whiteboard: | needtest | ||||||||
| Attachments: |
|
||||||||
Initial analysis: Sirius has several sub-systems which handle the session state (is it clean or dirty?), and this is a case where they get different ideas about what the current state is, causing confusion and inconsistencies. What happens normally in a normal situation, starting from a clean state: 1. Some model is modified inside an EMF Transaction. A notification is sent to the TransactionChangeRecorder installed on the element. 2. Our ResourceModifiedFieldUpdater#handleResourceChange(Resource, Notification) is also notified of the change, as we have installed it as the TED's ValidateEditSupport implem. It takes note that the resource containing the modified element is changed, but does nothing more at this point. 3. The transaction finishes, and during the commit() our ResourceModifiedFieldUpdater.finalizeForCommit() is called. It calls Resource.setModified(true) on every resource touched during the tx, and clears its list of modified resources. At this point all Resources where actual modifications happens are marked as modified, from the point of view of Resource.isModified. 4. We then run the various post-commit listeners. One of them is the ResourceSetSync, which has its own notion of the status of various resources (in particular they are SYNC initialy, CHANGED when seen as modified). ResourceSetSync analyzes the changes which occured during the tx, and (using slightly different and more complex criterion than ResourceModifiedFieldUpdater) computes the new resource state. After a tx which changed the model, the new state is CHANGED. If the new state is different than what ResourceSetSync remembered from the last time, and only in this case (i.e. only on the SYNC -> CHANGED transition, but not on a CHANGED -> CHANGED case), it notifies its listeners. 5. Each session has a SessionResourcesSynchronizer registered as client for the ResourceSetSync, so we get notified by a call to SessionResourcesSynchronizer.statusesChanged(), with a description of the resources state change. For resources which have become CHANGED (from the ResourceSetSync point of view), we notify the session's own listeners: session.notifyListeners(SessionListener.DIRTY). So far so good. 6. Then, still in SessionResourcesSynchronizer.statusesChanged(), we call SessionResourcesSynchronizer.allResourcesAreInSync() to determine the global state of the session, and call either session.setSynchronizationStatus(SyncStatus.SYNC) or session.setSynchronizationStatus(SyncStatus.DIRTY) depending on the result. allResourcesAreInSync() combines the resources' status as seen from the ResourceSetSync with their Resource.isModified() flag. In a normal case, both agree, at least on resource is seen as "not in sync", and the whole session is marked as SyncStatus.DIRTY. In the case of this bug, the only actual model modifications that occured during the transaction are reorderings of model elements in ELists. These do not trigger a call to ResourceModifiedFieldUpdater#handleResourceChange(Resource, Notification) (step 2 above), so the resource is not marked as modified at step 3. ResourceSetSync uses a diffrent approach to detect changes, so it correctly sets its own state for the resource as CHANGED (step 4). When we call allResourcesAreInSync() at step 6, because Resource.isModified returns false, we return true, and the session as a whole is marked SYNC. I do reproduce the 7 first steps but not 8 one:
> 8. Open the semantic model in the basic EMF editor: the attributes are in the initial order a1, a2, even though you just saved what looked like a state with the order a2, a1 (KO).
In my case, the A attributes were correctly displayed in the EMF editor (a2,a1)
For information, the problem is also visible with the scenario of bug #481433 comment 3. More analysis needed to confirm, but it looks like the root cause is org.eclipse.emf.transaction.impl.TransactionChangeRecorder.notifyChanged(Notification), which does not forward nottifications of type Notification.MOVE to the TED's ValidateEditSupport: see https://git.eclipse.org/c/emf-transaction/org.eclipse.emf.transaction.git/tree/org.eclipse.emf.transaction/src/org/eclipse/emf/transaction/impl/TransactionChangeRecorder.java#n186 If the only changes in a given transaction which impacts a resource are position changes in ELists, they will not be seen, and the resource's modified state becomes out-of-sync with ResourceSetSync's ideas of the file's status. The corresponding change was introduced a very long time ago (2006!) by commit e0ab1c36a4177595a544da93efc87d5ff0177fc6 for https://bugs.eclipse.org/bugs/show_bug.cgi?id=137503. That ticket mentions some known side-effects of this, but I'm not sure this one was intended. The bug will probably get fixed directly in EMF Transaction (for Oxygen), but I'll try to reproduce it outside of Sirius in plain EMF Tx before that. New Gerrit change created: https://git.eclipse.org/r/94463 Gerrit change https://git.eclipse.org/r/94463 was merged to [master]. Commit: http://git.eclipse.org/c/emf-transaction/org.eclipse.emf.transaction.git/commit/?id=03eed78097c7e29b80d91f55a4fbf29b38f8900d Created attachment 268153 [details]
Simple test to expose the bug
Attached is a plain EMF Tx test which exposes the bug. If time permits I'll rewrite it to integrate directly in the EMF Tx test suite, otherwise it will go in Sirius (with a version check on EMF Tx to avoid running on older versions where we know the bug exists).
Marking as resolved but "needtest". The fix was made in EMF Transaction, so we must wait for the availability of Oxygen M7-based builds to validate this properly. Verified on an Oxygen M7 based build. Available in Sirius 5.0.0, see https://wiki.eclipse.org/Sirius/5.0.0 for details. |
Created attachment 264660 [details] Sample project to reproduce the bug Steps to reproduce: 1. Import the attached project. 2. Open the diagram named "new Missed Dirty 2" (the bug exists on the other diagram but is slightly less visible there). 3. Note that in the semantic model (visible in the model explorer) and on diagram, atttibutes of class A are in the order a1, a2. Note also that the session is clean. 4. Select the "Swap attributes" tool in the palette and apply it to class A (not the attibutes). 5. In the model explorer, attributes are now in the order a2, a1, both in the Model Explorer and on the diagram. However the editor is *not* marked dirty! (KO). 6. Move the class on the diagram => the diagram is still not dirty. 7. Select the diagram background, and the "Save" and "Save All" actions become enabled in the toolbar, even though the diagram is still not dirty (KO). Click on "Save All". 8. Open the semantic model in the basic EMF editor: the attributes are in the initial order a1, a2, even though you just saved what looked like a state with the order a2, a1 (KO).