Refactoring client deregister listener behaviour #17646

sancar · 2020-09-30T11:17:59Z

We were returning false as the return value if we were not
successuflly deregister from any member and events was able to
continue to delivered for non deregistered members.

We have changed the behaviour so that we return true if a
registration is found always. And after this point, user will not
get any event. We will cleanup all the local handlers right away
to make sure of that.

Secondly we have set invocation timeout as infinite so that the
deregistartion from a connection is retried as long as the
connection is there until it is succesful.

I have left logging the exception when there is an unexpected
failure. We do not expect any but this is to be able to diagnose
if the unexpected happens.

We were returning `false` as the return value if we were not successuflly deregister from any member and events was able to continue to delivered for non deregistered members. We have changed the behaviour so that we return `true` if a registration is found always. And after this point, user will not get any event. We will cleanup all the local handlers right away to make sure of that. Secondly we have set invocation timeout as infinite so that the deregistartion from a connection is retried as long as the connection is there until it is succesful. I have left logging the exception when there is an unexpected failure. We do not expect any but this is to be able to diagnose if the unexpected happens.

puzpuzpuz

LGTM. Left some nits.

...ast/src/main/java/com/hazelcast/client/impl/spi/impl/listener/ClientListenerServiceImpl.java

ihsandemir

is it possible to write a test for the unhappy path?

sancar · 2020-10-05T08:29:37Z

@ihsandemir We have tests for failure scenarios like terminate , heartbeat timeout etc in AbstractListenersOnReconnectTest.

Listener service was waiting for the result of the future in a blocking way and, returning the registration id string. This is not in line with our other APIs in which we provide non-blocking API. Also, the slight behaviour change applied to Java client is ported to Python client. hazelcast/hazelcast#17646

* Make ListenerService return Future Listener service was waiting for the result of the future in a blocking way and, returning the registration id string. This is not in line with our other APIs in which we provide non-blocking API. Also, the slight behaviour change applied to Java client is ported to Python client. hazelcast/hazelcast#17646 * add missing .blocking() calls

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix hazelcast#17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes hazelcast#19269

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix #17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes #19269

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix hazelcast#17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes hazelcast#19269 (cherry picked from commit 1a5baf9)

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix hazelcast#17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes hazelcast#19269 (cherry picked from commit 1a5baf9) (cherry picked from commit 95d9d4b)

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix #17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes #19269 (cherry picked from commit 1a5baf9)

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix #17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes #19269 (cherry picked from commit 1a5baf9) (cherry picked from commit 95d9d4b)

It turns out that there are some services relying on the removal of a listener on the member when listener is removed from the client. Example: Cache listener count is kept per proxy. When listener is removed, we decrease the listener count from the cache context. Since this operation is async, it could be the case that the cache is destroyed from the calling thread before we decrement the value. In such cases, we decrement but the incremented value is lost. Therefore, we can end up with negative values. The listener derregistration invoctions are not waited. It was sync but we have changed the behavior as a side affect during this fix hazelcast#17646 We may solve this only for CacheListener but there could be more services relying on sync listener removal. Therefore, as a fix, we wait for invocation answer from the remote. fixes hazelcast#19269

sancar added Type: Defect Team: Client labels Sep 30, 2020

sancar added this to the 4.1 milestone Sep 30, 2020

sancar requested a review from a team as a code owner September 30, 2020 11:17

sancar self-assigned this Sep 30, 2020

sancar force-pushed the fix/deregisterListener/master branch from 54da2f0 to b7169be Compare September 30, 2020 14:34

puzpuzpuz approved these changes Oct 1, 2020

View reviewed changes

addressing review comments

315e001

ihsandemir approved these changes Oct 5, 2020

View reviewed changes

sancar merged commit 2cc3038 into hazelcast:master Oct 5, 2020

sancar deleted the fix/deregisterListener/master branch October 5, 2020 08:30

mmedenjak added the Source: Internal PR or issue was opened by an employee label Oct 5, 2020

puzpuzpuz mentioned this pull request Oct 5, 2020

[FORWARD-PORT] Improve listener deregistration hazelcast/hazelcast-nodejs-client#626

Merged

hazelcast deleted a comment Oct 5, 2020

mdumandag mentioned this pull request Nov 20, 2020

Make ListenerService return Future hazelcast/hazelcast-python-client#248

Merged

puzpuzpuz mentioned this pull request Nov 30, 2020

Improve listener deregistration logic hazelcast/hazelcast-nodejs-client#627

Merged

sancar added the All Languages Should Check Used by clients team to track fixes on the java client that should potentially backported to others label Jan 20, 2021

sancar mentioned this pull request Aug 9, 2021

Remove listeners sync with the calling thread #19289

Merged

This was referenced Aug 16, 2021

Remove listeners sync with the calling thread #19331

Merged

Remove listeners sync with the calling thread #19332

Merged

Remove listeners sync with the calling thread #19333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring client deregister listener behaviour #17646

Refactoring client deregister listener behaviour #17646

sancar commented Sep 30, 2020

puzpuzpuz left a comment

ihsandemir left a comment

sancar commented Oct 5, 2020

Refactoring client deregister listener behaviour #17646

Refactoring client deregister listener behaviour #17646

Conversation

sancar commented Sep 30, 2020

puzpuzpuz left a comment

Choose a reason for hiding this comment

ihsandemir left a comment

Choose a reason for hiding this comment

sancar commented Oct 5, 2020