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

fix(tiny_fd): assert disconnection on (re-)connection request of a co… #38

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

Conversation

chenlijun99
Copy link
Contributor

@chenlijun99 chenlijun99 commented Jun 19, 2022

This may happen for example if the peer was reset and right after boot it sends the SABM or SNRM frame. If the startup time of the peer is short (in the worst case it is enough that the startup time is less than the keep-alive timeout), it is possible that the primary has not detected the disconnection of peer yet.

Fixes #36

…nnected peer

This may happen for example if the peer was reset and right after boot
it sends the SABM or SNRM frame. If the startup time of the peer is
short (in the worst case it is enough that the startup time is less than
the keep-alive timeout), it is possible that the primary has not
detected the disconnection of peer yet.
@lexus2k
Copy link
Owner

lexus2k commented Jul 18, 2022

I'm sorry for so very long delay.
The fix confused me that it doesn't take into account all possible states, when communication is considered active.
It seems that DISCONNECTING state should be also use this new branch.
I need to test it

@chenlijun99
Copy link
Contributor Author

Thanks for the reply. Honestly, I think I should have included some unit tests, but the existing testing fixtures are rather unfamiliar to me. If you are so kind to test and validate this patch, that would be awesome! Alternatively, I'll take some time to add the tests, but I can't promise anything in terms of time.

@chenlijun99
Copy link
Contributor Author

chenlijun99 commented Jul 18, 2022

BTW

It seems that DISCONNECTING state should be also use this new branch.

Seems reasonable to me.

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.

Connection is silently re-established
2 participants