vaadin / collaboration-engine

The simplest way to build real-time collaboration into web apps
https://vaadin.com/collaboration
Other
3 stars 1 forks source link

Closing last collaboration-binder topic does not clear collaboration-engine state #23

Closed hirschr closed 3 years ago

hirschr commented 3 years ago

If an user leaves / removes the connection to a topic it remains in the collaboration engines state (ap of topics) including the complete topics data map. And there is no acces to the active topic connection counter nor the possibility to set a custom topic activation handler in order to react to topic counter state change.

If theres is no connection left to a specific topic it should be (re)intitialized when the next connection is openened to that topic.

To achive this there are three possible solutions:

  1. collaboration engine removes the topic from the topics map if last connection has been closed
  2. collaboration engine offers the possibility to provide a custom topicActivationHandler
  3. collaboration engine offers access to the connection counter for a specific topic id (activeTopicsCount), so we can clear the topics data map depending on that counter
Legioth commented 3 years ago

One potential challenge to take into account here is that the state should most likely not be reset immediately when the last user leaves since they might just e.g. have to restart their computer for some reason and would expect their incomplete edits to remain when they come back regardless of whether someone else has also been present while they were rebooting.

Instead, there would have to be some kind of timeout, which might even be contextual. From that point of view I think should Collaboration Engine provide a callback that application code can hook in to rather than adding any actual decision logic to Collaboration Engine itself.

Legioth commented 3 years ago

After going back and forth a while with the rest of the team, we have concluded that anything based on callbacks registered by application code would become complicated in combination with the upcoming clustering functionality since you'd want those callbacks to only be run once for the whole cluster rather than once per user or once per application server.

Instead, we'd suggest a low level feature to mark any CollaborationMap for deletion whenever the topic it belongs to has been without connections for a configurable duration. There would also be a high level shorthand API in CollaborationBinder.

The API in both cases would be similar:

public void setInactiveClearTimeout(java.time.Duration timeout);
public Optional<java.time.Duration> getInactiveClearTimeout();

Setting the timeout to 0 means that the data is cleared immediately when the last user disconnects. Setting the timeout to null (which is the default) will disable the feature so that no automatic clearing is performed. If multiple configurations are set concurrently from multiple threads, then the last write wins.

When used on CollaborationMap, all data related to that map will be removed, including the setting it self. This means that it needs to be set again every time a connection is opened. When used on a CollaborationBinder, the setting will automatically be applied again if the same CollaborationBinder instance becomes connected again (because the binder applies it to its underlying map every time it connects).

As an implementation detail, the data might actually be cleared only the next time a connection is opened to the topic rather than from a timer at the exact moment when the timeout expires. This is because we want to avoid the complexity from the multiple different ways needed to properly manage background threads in different environments (i.e. Spring schedulers or Java EE managed executors). The difference should be indistinguishable from the application's point of view since the data is cleared before the next topic connection can observe any old values, but it might lead to some redundant garbage that could otherwise be collected.

@hirschr How does this approach sound to you?

hirschr commented 3 years ago

For me the approach marking a CollaborationMap with a deletion timeout after becoming inactive would be just fine. Infact it meets the first suggested solution (collaboration engine removes the topic from the topics map if last connection has been closed) and adds the possibility to configure the behaviour per Binder / Map.

Just to be sure I got your remark (data might actually be cleared only the next time a connection) right: As a result of closing all connections to a topic, calling CollaborationBinder::setTopic on a topic that has been disconnected (and timed out), the CollaborationBinder should initialize field bindings from initialBeanSupplier.

Legioth commented 3 years ago

Could also mention that the reason for doing the deletion per map instead of for a whole topic is that we're anticipating situations when the same topic id might be used both for a form and for e.g. discussion related to the same entity. In that case, the map containing form data should be cleared after inactivity whereas the discussion data should be retained.

Just to be sure I got your remark (data might actually be cleared only the next time a connection) right: As a result of closing all connections to a topic, calling CollaborationBinder::setTopic on a topic that has been disconnected (and timed out), the CollaborationBinder should initialize field bindings from initialBeanSupplier.

Yes, CollaborationBinder should check for existing contents in the map whenever a connection is opened and initialBeanSupplier is run if no data is there.

I could also mention that calling setTopic isn't enough to trigger a connection - at least one field component managed by the binder also needs to be attached to a UI. This does usually not matter since both conditions typically happen during the same round trip, but it still something that could cause surprises in some special cases (e.g. eagerly initialized content for a dialog that isn't yet opened).

pekam commented 3 years ago

Notes from team discussion.

Our latest suggestion for the API in CollaborationMap is:

public void setExpirationTimeout(java.time.Duration timeout);
public Optional<java.time.Duration> getExpirationTimeout();

Implementation details: This could be achieved by adding the following fields to Topic:

Map<String, Duration> mapExpirationTimeouts;
Instant lastDisconnected; // null when connectionCount != 0
int connectionCount;

For testing, we'd want to mock the current time. We are already mocking the current date for the monthly-active-users-tests. There will be a separate task to give CollaborationEngine a Clock instance to unify the ways we are mocking time for our unit tests.

pekam commented 3 years ago

The setExpirationTimeout API has been implemented in CollaborationMap. We're expecting it to be released in a pre-release 3.1.0.alpha1 in the near future. If you could then test the feature and provide your feedback, that would be highly appreciated!

Here's a follow-up ticket for adding a convenience API for CollaborationBinder: #24.

hirschr commented 3 years ago

Thanks a lot. I will test your Implementation as soon as 3.1.0.alpha1 is available.best regards, Robert--Diese Nachricht wurde von meinem Android Mobiltelefon mit GMX Mail gesendet.Am 22.01.21, 10:12 schrieb "Pekka Maanpää" notifications@github.com:

The setExpirationTimeout API has been implemented in CollaborationMap. We're expecting it to be released in a pre-release 3.1.0.alpha1 in the near future. If you could then test the feature and provide your feedback, that would be highly appreciated! Here's a follow-up ticket for adding a convenience API for CollaborationBinder: #24. —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

pekam commented 3 years ago

Great to hear!

I'll cross-post here the usage example that I wrote in #24:

CollaborationEngine.getInstance().openTopicConnection(this, "topic-id",
        userInfo, topicConnection -> {
            topicConnection
                    .getNamedMap(CollaborationBinder.class.getName())
                    .setExpirationTimeout(Duration.ofMinutes(15));
            return null;
        });

It's really verbose for now, but it will be get better when we implement #24.

If you want the field values to reset immediately when the last collaborator leaves, replace Duration.ofMinutes(15) with Duration.ZERO.

Peppe commented 3 years ago

3.1.alpha1 is now released. Release notes are here: https://github.com/vaadin/collaboration-engine/releases/tag/3.1.0.alpha1

Note that it is a pre-release, so it is in our pre-release repository.

pom.xml:

<repositories>
  <repository>
    <id>vaadin-prereleases</id>
    <url>https://maven.vaadin.com/vaadin-prereleases</url>
  </repository>
</repositories>