Don't remove subscriptions while iterating them #2157
Conversation
server/src/main/java/io/deephaven/server/appmode/ApplicationServiceGrpcImpl.java
Outdated
Show resolved
Hide resolved
niloc132
force-pushed
the
2070-dont-remove-in-loop
branch
from
7257c1a
to
a80b6cb
Compare
| @@ -209,7 +210,7 @@ private synchronized void propagateUpdates() { | |||
| updatedFields.clear(); | |||
| final FieldsChangeUpdate update = builder.build(); | |||
|
|
|||
| subscriptions.forEach(sub -> sub.send(update)); | |||
| subscriptions.removeIf(subscription -> !subscription.send(update)); | |||
There was a problem hiding this comment.
Do we need any of the side-effects that onClose previously provided? (We aren't calling onClose now.)
session.removeOnCloseCallback(subscription)subscription.notifyObserverAborted()
There was a problem hiding this comment.
No you're right, though the broken behavior won't be visible externally
- the leaked callback will just leak until the session cleans it up, which isnt ideal
- the observer notification doesnt matter, since the observer is already by definition failed
I really dont care for this close-is-remove-but-not-cancel pattern we have used here and elsewhere. I am also not sure that what we seem to have here makes sense at all (why does a close() call necessarily mean sending the aborted message, but doesnt do the cleanup from the session?), but I think that cleanup is outside the scope for this set of changes.
niloc132
force-pushed
the
2070-dont-remove-in-loop
branch
from
4270324
to
24d06c0
Compare
| // must be sync wrt parent | ||
| /** | ||
| * Sends an update to the subscribed client. Returns true if successful - if false, the client is no longer | ||
| * listening and this subscription should be canceled after iteration. |
There was a problem hiding this comment.
Looking at callers of send - do we also need to do an onCancel call in listFields?
There was a problem hiding this comment.
Probably, though a) it is a weak ref anyway, and b) the subscription won't be in the list so doesnt need to be removed. I'll make the change in this patch, though it probably should be incorporated into a bigger change that generally cleans up these patterns, rather than fixing each piecemeal as we've been doing.
niloc132
force-pushed
the
2070-dont-remove-in-loop
branch
from
24d06c0
to
aa50bee
Compare
Fixes #2070