Skip to content

[ISSUE #4144] Subscription is almost impossible to be cancelled when the TCP sub client is closed #4145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
xwm1992 merged 4 commits into from
Jun 28, 2023
Merged

[ISSUE #4144] Subscription is almost impossible to be cancelled when the TCP sub client is closed #4145

xwm1992 merged 4 commits into from
Jun 28, 2023

Conversation

@pandaapo
Copy link
Member

@pandaapo pandaapo commented Jun 20, 2023 •
edited
Loading

Fixes #4144.

Motivation

Subscription is almost impossible to be cancelled when the TCP sub client is closed.

Modifications

eventmesh/eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/core/protocol/tcp/client/group/ClientGroupWrapper.java

Line 246 in 8843df2

r = topic2sessionInGroupMapping.get(topic).remove(session);

The subscription session can't be removed from ClientGroupWrapper.topic2sessionInGroupMapping correctly.

When Session is added to the topic2sessionInGroupMapping, a java.util.Set, Session.sessionState is CREATED. But session is almost impossible to keep this state. It will be RUNNING or CLOSED. So the hash of Session is changed. Map(the base of Set) will be unable to find the bucket using the new hash, which is different from origin hash used when initially putting into Map.

Documentation

  • Does this pull request introduce a new feature? no

@codecov
Copy link

codecov bot commented Jun 26, 2023 •
edited
Loading

Codecov Report

Merging #4145 (84a2d1d) into master (991cbda) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 84a2d1d differs from pull request most recent head 6c3d252. Consider uploading reports for the commit 6c3d252 to get more accurate results

@@             Coverage Diff              @@
##             master    #4145      +/-   ##
============================================
- Coverage     16.93%   16.93%   -0.01%     
  Complexity     1410     1410              
============================================
  Files           588      588              
  Lines         25739    25741       +2     
  Branches       2387     2387              
============================================
  Hits           4358     4358              
- Misses        20946    20948       +2     
  Partials        435      435
Impacted Files Coverage Δ
.../admin/handler/ShowListenClientByTopicHandler.java 12.90% <0.00%> (ø)
.../protocol/tcp/client/group/ClientGroupWrapper.java 0.00% <0.00%> (ø)
...time/core/protocol/tcp/client/session/Session.java 2.34% <0.00%> (-0.06%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lrhkobe
Copy link
Contributor

lrhkobe commented Jun 27, 2023

Thank you for your problem. I have checked this issue, because the reference is stored in the set, so there is no problem. You can run and verify it.

@pandaapo
Copy link
Member Author

pandaapo commented Jun 27, 2023 •
edited
Loading

@lrhkobe Thank you for your review.

Set indeed stores Session's reference. But it does not mean Set<Session>#remove(Session) can remove a stored Session successfully. Because Session overrides the hashCode() method, it results in the hash value of Session will be changed after it is stored in Set. Just like what I explain in #4145 (comment)

When Session is added to the topic2sessionInGroupMapping, a java.util.Set, Session.sessionState is CREATED. But session is almost impossible to keep this state. It will be RUNNING or CLOSED. So the hash of Session is changed.

Set#remove uses Map#remove to implement. Map use the hash value to find an element to remove. Because the hash value is changed after Session is put into Map's bucket, Map#remove will fail even if the object being removed is the same object as one of the objects in the Map, aka they have the same reference.

I had run the eventmesh-examples just like what I wrote in #4144 (comment). Then found this problem.

lrhkobe
lrhkobe approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@lrhkobe lrhkobe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xwm1992 xwm1992 merged commit 1600c6b into apache:master Jun 28, 2023
xuhongjia pushed a commit to Deckers-Ohana/eventmesh that referenced this pull request Mar 13, 2025
@pandaapo
[ISSUE apache#4144] Subscription is almost impossible to be cancelled…
a230a77 
… when the TCP sub client is closed (apache#4145)

* Fix bug: suscription session can't be removed correctly.

* Fix bug: suscription session can't be removed correctly.

* remove the modification for other issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xwm1992 xwm1992 xwm1992 approved these changes

@lrhkobe lrhkobe lrhkobe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Subscription is almost impossible to be cancelled when the TCP sub client is closed

3 participants

@pandaapo @lrhkobe @xwm1992