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.
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
Add SDK Router message handling #316
Add SDK Router message handling #316
Changes from 13 commits
91c34d2
4baa94d
b6aac63
78420bf
28ad9f8
b10e5dc
96b7b09
5154ca3
1b32de6
f5d8a0d
09126b4
d2e6c19
215e315
c3287fe
3328e14
65935bd
72f7ab1
e06646e
6ad4f35
d332ad7
60de385
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since
closed
is atomic, could we move this lock to directly beforehandler, exists := n.markRequestFulfilled(requestID)
?Should we move it to inside
markRequestFulfilled
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes much more sense than what I was currently doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very subtle change. So we should be terrified to make it.
I believe we must hold the lock when checking for
n.closed
on all of the inboundresponse
+requestFailed
flows to avoid a potential panic due to a write to a closed channel (or closing a channel twice).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline I'll see if I can make a separate regression test for this invariant as a follow-up to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I documented the invariant on
n.closed
as well 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we okay with dropping responses (and timeouts) that should have been sent to the SDK router after we close the network?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not okay with this - then I think we'll need to be fairly careful around what gets passed through to the router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We chatted offline but agreed it was cleanest to leave the
Router
code as-is, and just drop theclosed
check on the server-end because we're guaranteed that the outstanding request is empty on shutdown because we empty it onShutdown
and stop sending requests after the flag is set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was written to ensure that an invalid message NEVER triggers an unintentional fatal error, so it seems a bit weird to change it in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was unsure if it made more sense to just remove these test cases or just test for the sdk error. I guess now the property that we have is that invalid messages are always forwarded into the router, so we can either check for this error, get rid these tests, or write a
Router
interface thatp2p.Router
implements but maybe that's overkill.