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

add test for deadlines #60

Merged
merged 2 commits into from
May 23, 2019
Merged

add test for deadlines #60

merged 2 commits into from
May 23, 2019

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented May 22, 2019

seems to be broken...

@vyzo vyzo requested a review from Stebalien May 22, 2019 22:38
@Stebalien
Copy link
Member

Deadlock?

@vyzo vyzo changed the title [WIP] fix deadlines for FullClose fix deadlines for FullClose May 22, 2019
@vyzo
Copy link
Contributor Author

vyzo commented May 22, 2019

Not sure what it is, the AwaitEOF goroutines are stuck in waitForData apparently without a deadline.

@Stebalien
Copy link
Member

Can we figure out what's actually happening? The code looks correct.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

I agree that the code looks correct, so this is a little mystifying...

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

Oh I think I know what's happening: we set the handshake deadline in the stream, which can then time out, killing the Close sendMsg!
It's a bug in the relay code, the deadline needs to be reset before attempting to handleError.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

No wait, close is not sent with the deadline.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

ah, it is the bug indeed in the error message is never sent due to the deadline being exceeded, which results in the initiating peer never responding.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

no wait, the deadline is on the other side of the hop.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

trying to debug the issue: restored deadline setting checks and added error logs when not setting the read deadlines.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

It's full of these:

May 23 10:20:59 relays-sjc-01 relay.sh[18486]: 10:20:59.374 ERROR      mplex: not setting deadline; stream is closed stream.go:255

and these:

May 23 10:20:59 relays-sjc-01 relay.sh[18486]: 10:20:59.380 ERROR      mplex: not setting read deadline; remote has closed stream.go:262

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

so the stream is already marked closed, failing to set the deadline in AwaitEOF.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

the more pertinent question is why waitForData doesn't return immediately, which would be the bug.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

I don't understand how this is happening, we always close either reset or dataIn when we set closedRemote = true...

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

Added assertion in c2ceeb7 to test for the impossible condition.

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

it's not triggering, so that was possibly a red herring...

@vyzo
Copy link
Contributor Author

vyzo commented May 23, 2019

I can't seem to reproduce the issue; I don't know what happened.

At any rate, this adds a test for deadlines that was missing.

@vyzo vyzo changed the title fix deadlines for FullClose add test for deadlines May 23, 2019
@Stebalien Stebalien merged commit b10cf5e into master May 23, 2019
@Stebalien Stebalien deleted the fix/deadlines branch May 23, 2019 16:29
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.

2 participants