Connection manager transition order test using IOSimPOR #3640
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
coot
added
connection-manager
coot
changed the title
bolt12
reviewed
Mar 10, 2022
Comment on lines
+1660
to
+1693
| case connState of | ||
| ReservedOutboundState -> do | ||
| let connState' = UnnegotiatedState provenance connId connThread | ||
| writeTVar connVar connState' | ||
| return ( Just $ mkTransition connState connState' | ||
| , Nothing | ||
| ) | ||
| TerminatingState {} -> | ||
| return (Nothing, Nothing) | ||
| TerminatedState {} -> | ||
| return (Nothing, Nothing) | ||
| _ -> | ||
| return ( Nothing | ||
| , Just (CM.TrUnexpectedlyFalseAssertion | ||
| (RequestOutboundConnection | ||
| (Just connId) | ||
| (abstractState (Known connState)) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
| traceWith trTracer (TransitionTrace peerAddr trans) | ||
| traverse_ (traceWith trTracer . TransitionTrace peerAddr) trans | ||
| traverse_ (traceWith tracer >=> evaluate . assert True) | ||
| mbAssertion |
There was a problem hiding this comment.
Nice, is there an open issue for this found bug?
There was a problem hiding this comment.
No, instead I just added io-sim-discovered flag to this PR, there's just one bug which is described in the commit so it's should be enough.
Using *IOSimPOR*.
When we fork outbound connection handler we need to update the state to `UnnegotiatedState`, but we should not do that blindly. It could happen that the connection broke and was put in `TerminatedState` (or `TerminatingState`) in the meantime by the connection handler thread. This bug was discovered by *IOSimPOR*.
coot
force-pushed
the
coot/cm-io-sim-por-2
branch
from
44f0d8c
to
4a0b181
Compare
|
bors merge |
|
Build succeeded: |
coot
added a commit
that referenced
this pull request
May 16, 2022
3640: Connection manager transition order test using IOSimPOR r=coot a=coot This PR adds a quickcheck property test which validates order of connection manager transitions using *IOSimPOR*. It also fixes a connection manager bug discovered thanks to the new test. Fixes #3613. - connection-manager-tests: test transition order - connection-manager: don't blindly override state - connection-manager-tests: code style 3662: Removed ST effects from IOSimPOR r=coot a=coot ST effects are thread local, they never race with other steps. This patch reverts a previous fix commited in PR #3647. Thanks for `IOSimPOR` we also found out that the transition: `UnnegotiatedSt Inbound → TerminatingSt` should be valid, similarly to one for outbound connections (see PR #3652, commit fbcb6aa). Co-authored-by: Marcin Szamotulski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a quickcheck property test which validates order of connection manager transitions using IOSimPOR. It also fixes a connection manager bug discovered thanks to the new test.
Fixes #3613.