ModelUpdatedProvider's selfListener results in a surprising lifecycle

[eclipse-viatra-bot]

| --- | --- | | Bugzilla Link | 543794 | | Status | NEW | | Importance | P3 normal | | Reported | Jan 24, 2019 11:29 EDT | | Modified | Feb 07, 2019 03:29 EDT | | Version | 2.1.0 | | See also | Gerrit change https://git.eclipse.org/r/136201, Git commit 64d9c085, Gerrit change https://git.eclipse.org/r/136259, Git commit 2a0e684f | | Reporter | Zoltan Ujhelyi |

Description

The ModelUpdateProvider of a ViatraQueryEngine instance registers a lifecycle listener on its on engine to remove all change listeners from the engine on dispose.

However, this causes surprising log entries in the output if a transformation is disposed during engine disposal:

ERROR org.eclipse.viatra.query.patternlanguage.emf.EMFPatternLanguageRuntimeModule.runtime.org.eclipse.viatra.query.runtime.api.ViatraQueryEngine.1940867248 - Listener org.eclipse.viatra.transformation.evm.update.QueryEngineUpdateCompleteProvider$ModelUpdateListener@42b54862 already removed from map (e.g. engine was already disposed)!

This was caused by the following scenario:

1. The transformation registered update listeners and a lifecycle listener to dispose the transformation when the engine is disposed.
2. The engine was disposed.
3. At first, the selfListener of ModelUpdateProvider was called, clearing the listener list.
4. Then the transformations lifecycle listener was called, starting the disposal of the transformation.
5. During the disposal, it tried to remove the corresponding model update listeners, but they were already cleared in step 3. At this point the previously mentioned log entry was provided.\ \ This is surprising, as during the disposed() callback I would expect to be able to remove the entries without errors, but I am not allowed.\ \ Possible solutions:\ a) Instead of relying on a lifecycle listener the engine should provide the notification directly to the ModelUpdateProvider after the dispose listeners are called. This would allow users to normally dispose their listeners, without the log messages.\ b) The internal lifecycle listener (or more if there are others) could be considered of lower priority - first the external users can shut down, and only after that point the internal ones, resulting in the same result.\ c) Explicitly documement that the disposed() method is called after the disposal is complete, and provide another callback before disposal.\ \ Gabor, Abel: do you have any preferences about these approaches? Or do you remember some discussion that is the cause of this issue? Thanks for the feedback.

[eclipse-viatra-bot]

By Abel Hegedus on Jan 24, 2019 12:26

(In reply to Zoltan Ujhelyi from comment #0)

Explicitly document that the disposed() method is called after the disposal is complete

It is called 'disposed' for the very reason to convey that it is called after the disposal occurred.

On a more constructive note: so far we have anticipated that the transformation developer is in control of the engine and will dispose the transformation before disposing the engine.

In addition to suggestion a), we could make the ModelUpdateProvider more robust, similarly to ViatraQueryEventSource in 889f50ce319cdc312354d78b60e16de8b8cf7ff0 (by checking whether the engine has already been disposed).

I also agree with c), extending the lifecycle listener interface with beforeX methods is OK, just make sure to catch all exceptions from listeners to ensure disposal happens eventually.

[eclipse-viatra-bot]

By Zoltan Ujhelyi on Jan 25, 2019 03:27

About when 'disposed' is called: whether or not the ModelUpdateListeners external listeners were wiped is dependent on the ordering of lifecycle listeners, which is a bi strange, and looking at ViatraQueryEngineImpl#dispose these listeners are used to remove Base Index listeners before the the index is disposed... This is a bit strange, to be honest.

I agree that this is a new feature request, as we indeed did anticipate the transformation developer to be in control of the engine, but now we have a scenario where this is not the case, so we need some mechanism that still allows us to shut down everything correctly.

In its current form, ModelUpdateProvider does not have to check whether the engine is disposed as it receives a notification for it (and uses it to empty its listener list). What we could make more robust is the cleanup on the EVM side: if the engine is disposed, it can expect its listeners removed and it does not have to dispose them. Thinking about it more, this might be the solution for this issue, as this is generic, avoids the issue mentioned and does not require deep changes in the behavior of the lifecycles.

[eclipse-viatra-bot]

Feb 03, 2019 08:04

New Gerrit change created: https://git.eclipse.org/r/136201

[eclipse-viatra-bot]

Feb 04, 2019 13:28

Gerrit change https://git.eclipse.org/r/136201 was merged to [master].\ Commit: http://git.eclipse.org/c/viatra/org.eclipse.viatra.git/commit/?id=64d9c085bf9cb3652a334bab6b28c11119cc3148

[eclipse-viatra-bot]

Feb 04, 2019 15:07

New Gerrit change created: https://git.eclipse.org/r/136259

[eclipse-viatra-bot]

Feb 05, 2019 03:40

Gerrit change https://git.eclipse.org/r/136259 was merged to [2.1-maintenance].\ Commit: http://git.eclipse.org/c/viatra/org.eclipse.viatra.git/commit/?id=2a0e684f0319beee19349480fa638a8bd153f67a

[eclipse-viatra-bot]

By Zoltan Ujhelyi on Feb 07, 2019 03:29

A workaround for the original issue (checking the disposed state of the query engine before unregistering the change listeners) has been implemented in master and 2.1-maintenance branches. However, I am keeping this issue open, as we should clean up the handling of engine lifecycle listeners.