Make monitorPeerConnection non-blocking #4067
Merged
+162
−108
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
force-pushed
the
coot/peer-selection-action-fix
branch
from
6c535f1
to
cf6f9dd
Compare
coot
force-pushed
the
coot/peer-selection-action-fix
branch
from
cf6f9dd
to
c733e87
Compare
coot
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Oct 11, 2022
Includes the IntersectMBO/ouroboros-network#4067 fix.
coot
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Oct 11, 2022
Includes the IntersectMBO/ouroboros-network#4067 fix.
bolt12
approved these changes
Oct 12, 2022
Comment on lines
+215
to
+217
| -- | Monitor peer state. Must be non-blocking. | ||
| -- | ||
| monitorPeerConnection :: peerconn -> STM m (PeerStatus, ReconnectDelay), | ||
| monitorPeerConnection :: peerconn -> STM m (PeerStatus, Maybe ReconnectDelay), |
There was a problem hiding this comment.
If it must be non-blocking could we somehow get rid of STM?
There was a problem hiding this comment.
no because the Guarded monoid is executed in STM monad.
There was a problem hiding this comment.
Right, I had edited this comment don't know why it didn't get through. But I think this comment is misleading, we want monitorPeerConnection to be non-blocking in a particular function's logic. Saying this at the top level and then have a STM type is bit confusing. I'd be more accurate and explain in what situation we do not want to block
Comment on lines
756
to
764
| -> STM m (Map MiniProtocolNum (Maybe (HasReturned a))) | ||
| -- do not block when a mini-protocol is still running | ||
| f = traverse (((\stm -> (Just <$> stm) `orElse` pure Nothing))) | ||
| <=< readTVar . ahMiniProtocolResults |
There was a problem hiding this comment.
I believe this is the culprit? Maybe explain here with more detail why we do not want this to block.
There's no need to wake up the outbound-governor multiple times when the mini-protocols are terminating. Added comments.
`ReconnectDelay` ought to use `max` as its semigroup rather than `+`.
This allows to use fractional literals without wrapping them in `ReconnectDelay` constructor.
Instead of relaying on `policyErrorDelay` we can relay on the reconnect delay computed by `monitorPeerConnection`.
coot
force-pushed
the
coot/peer-selection-action-fix
branch
from
c733e87
to
332c303
Compare
This will cause the `peerSelectionGovernor` to fail and will stop the diffusion layer. This is better than silently skipping update of the state due to an asynchronous demotion.
bolt12
reviewed
Oct 13, 2022
ouroboros-network/src/Ouroboros/Network/PeerSelection/PeerStateActions.hs
Show resolved
Hide resolved
bolt12
approved these changes
Oct 13, 2022
|
bors merge |
coot
added a commit
to IntersectMBO/cardano-node
that referenced
this pull request
Oct 14, 2022
Includes the IntersectMBO/ouroboros-network#4067 fix.
This was referenced Oct 27, 2022
iohk-bors bot
added a commit
that referenced
this pull request
Nov 9, 2022
4120: Cherry picked network changes for cardano-node-1.35.5 release r=coot a=coot This cherry-picked patches from the following PRs: * #3794 * #3844 * #3785 * #3904 * #3915 * #3852 * #3970 * #3979 * #4015 * #4067 * #4004 * #4086 * #4113 * #4106 * #4127 * #4103 Also cherry-picked almost all the commits which modify GitHub actions: * 18c5244 Run GitHub Actions on pull requests * 3adf5a9 Use newer version of io-sim * ee9b7a6 Fix GH Actions Windows CI: switch from pkgconf to pkg-config * e6cf074 github-actions: use `ubuntu-latest` * 9a8b959 Updated versions of github actions * fc8f8f0 Fix GH Actions Windows CI caching * 7f07c40 Windows Github Actions now use MSYS2 * b21a7ce Fix chocolatey CI error * #4134 TODO: * [x] bump versions of packages * [x] input-output-hk/cardano-haskell-packages#84 Co-authored-by: Mark Tullsen <tullsen@galois.com> Co-authored-by: Marcin Szamotulski <coot@coot.me>
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.
Description
Fixes #4064
TODO:
improve network simulation test so it catches the problem(it will be done in another PR)Checklist
interface-CHANGELOG.mdinterface-CHANGELOG.md