-
Notifications
You must be signed in to change notification settings - Fork 20
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
use timeouts when sending messages for stream open, close, and reset. #52
Conversation
test failures are #47 |
We might also consider not sending a reset message at all, just swallow the remote error. |
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.
Looking at this closer, I think this patches the wrong sites. Calls to Reset()
and Close()
can still block forever. It seems that patching sendMsg
to fallback to a grace period iff the context has no deadline would cover all bases more accurately:
Lines 150 to 155 in dec60e0
dl, hasDl := ctx.Deadline() | |
if hasDl { | |
if err := mp.con.SetWriteDeadline(dl); err != nil { | |
return err | |
} | |
} |
Not the wrong sites, these sites clearly need to be patched. It's just not all of them.
We have no mechanism to propagate the error to the user in these cases, and I don't know what the semantics of failing to reset or close because of a timeout should be. |
Also patched the |
actually we probably need a timeout there, and return the error. |
We don't really take action on the error in our code, and at best we would reset the stream. |
it log spams otherwise
it spams otherwise
it spams a lot in relays
summoning @Stebalien @raulk -- this has been thoroughly tested, let's review and get it merged! |
it's a lock after all.
Closes #51 #48