-
Notifications
You must be signed in to change notification settings - Fork 10
Add timeout functionality when matching connections #10
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
base: master
Are you sure you want to change the base?
Conversation
84ad81e
to
b2175a1
Compare
@@ -20,6 +20,7 @@ import ( | |||
"io" | |||
"net" | |||
"sync" | |||
"time" | |||
) | |||
|
|||
// Matcher matches a connection based on its content. |
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.
Can you add a comment on how IO errors are handled?
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 the caller configures a timeout and it occurs that would mean cmux does not match the connect (i.e. it results in an ErrNotMatched)
So i've added comments to that error struct. Hopefully that is intuitive, but lmk if you'd prefer something else.
return ch | ||
} | ||
|
||
func TestServeWithTimeout(t *testing.T) { |
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.
Do you mind breaking this into two tests or convert it to table driven tests? I think that makes it easy to follow.
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.
Made the first test a bit smaller now that we can check the ErrorHandler directly.
Separated the two tests into subtests 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.
I added a couple of comments. Can you please take a look? Otherwise the changes look good.
b2175a1
to
8d52121
Compare
An issue in the cockroachdb repo (cockroachdb/cockroach#143785) uncovered an undesired behaviour in the goroutines handled by cmux. Particularly, cmux can hang forever on trying to match a connection. This can be resolved by users of cmux as illustrated in cockroachdb/cockroach#144170. However, it is an unorthodox solution. This commit adds functionality to cmux to optionally specify a timeout on matching (reading) connections.
8d52121
to
a54c864
Compare
Discussed offline w/ @cthumuluru-crdb Ideally, an error due to a timeout would be identified as so, i.e. in the implementation for However, this is tricky b/c the interface for the Matcher swallows the error
And if we change that type, we have to change consumers of cmux, for example: https://github.com/cockroachdb/cockroach/blob/master/pkg/server/start_listen.go#L123 In this PR we explored using a Similarly, if we tried reading from the connection again to determine if the timeout expired (read deadline was exceeded) after not matching, then we might end up blocking on that read for the timeout duration. So we have settled on implementing the timeout functionality but not identifying the timeout errors. |
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.
LGTM. Thank you!
An issue in the cockroachdb repo (cockroachdb/cockroach#143785) uncovered an undesired behaviour in the goroutines handled by cmux.
Particularly, cmux can hang forever on trying to match a connection.
This can be resolved by users of cmux as illustrated in cockroachdb/cockroach#144170. However, it is an unorthodox solution.
This commit adds functionality to cmux to optionally specify a timeout on matching (reading) connections.