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

remove unused error errAlreadyHandlingConnectionLoss #672

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wcsiu
Copy link

@wcsiu wcsiu commented Mar 31, 2024

Chanages

  • Remove unused errAlreadyHandlingConnectionLoss.

@wcsiu wcsiu changed the title remove unused error errAlreadyHandlingConnectionLoss remove unused error errAlreadyHandlingConnectionLoss and consider early disconnected case Mar 31, 2024
@MattBrittan
Copy link
Contributor

Sorry - I will be unable to accept this without a signed ECA (see the readme and the above link for more info).

The important bit of this PR would be || c.status == disconnected; however connectionStatus was designed such that transitioning from connecting to disconnected should not happen (if it does then there are potential race conditions). Consider the situation where the user calls Disconnect and then Connect again before connected gets called (seems possible from a quick review of the code).

However I believe that connectionStatus already accounts for this. Disconnecting() waits on c.actionCompleted before completing if the status was connecting or reconnecting (which should prevent the status from becoming disconnected before connected is called.

Now it is possible that I'm missing something (wrote this code a while back, it's quite complex, and I've just scanned it). Are you able to replicate the issue you are attempting to address?

@wcsiu
Copy link
Author

wcsiu commented Apr 10, 2024

Sorry - I will be unable to accept this without a signed ECA (see the readme and the above link for more info).

The important bit of this PR would be || c.status == disconnected; however connectionStatus was designed such that transitioning from connecting to disconnected should not happen (if it does then there are potential race conditions). Consider the situation where the user calls Disconnect and then Connect again before connected gets called (seems possible from a quick review of the code).

However I believe that connectionStatus already accounts for this. Disconnecting() waits on c.actionCompleted before completing if the status was connecting or reconnecting (which should prevent the status from becoming disconnected before connected is called.

Now it is possible that I'm missing something (wrote this code a while back, it's quite complex, and I've just scanned it). Are you able to replicate the issue you are attempting to address?

I think you are right about it that it will never reach disconnected state for that conditional check. My mistake on that.

@wcsiu wcsiu changed the title remove unused error errAlreadyHandlingConnectionLoss and consider early disconnected case remove unused error errAlreadyHandlingConnectionLoss Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants