Skip to content
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.

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

re-check reconnecting state before emitting #232

Conversation

kalinkostashki
Copy link
Contributor

@kalinkostashki kalinkostashki commented Oct 21, 2024

  • since the connectExecutor thread starts a reconnect which in turn causes the websocket connection to go into onConnected() method in the default(callback). This can cause an emit of message before reconnecting boolean is set to false
  • introduced a mechanism in order to make sure and retry several times to get the correct value

Fixes: #231

Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

@kalinkostashki thanks - I left some remarks. :)
I think this solution should work fixing the problem you observed - good catch. 👍

@kalinkostashki kalinkostashki force-pushed the bugfix/reconnecting-state-still-true-after-successful-reconnect branch from 176db08 to 336ba20 Compare October 22, 2024 11:22
@kalinkostashki
Copy link
Contributor Author

kalinkostashki commented Oct 22, 2024

@kalinkostashki thanks - I left some remarks. :) I think this solution should work fixing the problem you observed - good catch. 👍
@thjaeckle
done :)
I do have a question though:

for line 375 where I catch the InterruptedException I'm not quite sure how to handle it.
Should I rethrow it ? Or maybe interrupt the thread but that has its own implications?

@thjaeckle
Copy link
Member

@kalinkostashki thanks - I left some remarks. :) I think this solution should work fixing the problem you observed - good catch. 👍
@thjaeckle
done :)
I do have a question though:

for line 375 where I catch the InterruptedException I'm not quite sure how to handle it. Should I rethrow it ? Or maybe interrupt the thread but that has its own implications?

Yeah - normally the "correct" thing is to do Thread.currentThread().interrupt(); in the catch block for InterruptedException.
Should work also here, I assume.

@thjaeckle
Copy link
Member

@kalinkostashki do you want to add that Thread interruption?

@kalinkostashki kalinkostashki force-pushed the bugfix/reconnecting-state-still-true-after-successful-reconnect branch from 0f16d36 to 489a55f Compare October 28, 2024 08:38
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for the fix @kalinkostashki

@thjaeckle thjaeckle added this to the 3.6.1 milestone Oct 28, 2024
 - since the connectExecutor thread starts a reconnect which in turn causes the websocket connection to go into onConnected() method in the default(callback). This can cause an emit of message before reconnecting boolean is set to false
 - introduced a mechanism in order to make sure and retry several times to get the correct value

Signed-off-by: Kalin Kostashki <[email protected]>
@kalinkostashki kalinkostashki force-pushed the bugfix/reconnecting-state-still-true-after-successful-reconnect branch from 489a55f to 83b32b5 Compare October 28, 2024 09:50
@thjaeckle thjaeckle merged commit ca96adb into eclipse-ditto:master Oct 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working java
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Java] ClientReconnectingException thrown during reconnect dropping the subscriptionMessages
2 participants