don't use contexts for deadlines #58
Conversation
vyzo
force-pushed
the
fix/reuse-deadline-context
branch
from
c9d8ebc to
05c1acd
Compare
|
what the hell happened to travis? |
Needs to be enabled here: https://travis-ci.org/organizations/libp2p/repositories. I don't have permissions.
Could we try using https://github.com/libp2p/go-yamux/blob/master/deadline.go like we do in yamux? Then, we can just drop the contexts. |
ah yes, that might be good. |
|
Modified to use the pipeDeadline thingie. |
multiplex.go
Outdated
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-done: | ||
| return errors.New("invalid context") |
There was a problem hiding this comment.
? Shouldn't this be a timeout error?
There was a problem hiding this comment.
Yeah, good point. Is there an existing error we can reuse?
There was a problem hiding this comment.
Not that I know of. We just need an error that returns true for both Timeout() bool and Temporary() bool.
There was a problem hiding this comment.
I made an errTimeout, perhaps we want to export this (or reuse an existing error if applicable)
There was a problem hiding this comment.
ah, we need the Timeout and Temporary methods too, let me add.
| @@ -410,6 +412,8 @@ func (mp *Multiplex) handleIncoming() { | |||
|
|
|||
| msch.clLock.Unlock() | |||
|
|
|||
| msch.cancelDeadlines() | |||
There was a problem hiding this comment.
We need to encapsulate this more... but we can handle that later.
| defer s.deadlineLock.Unlock() | ||
| s.rDeadline = t | ||
| s.wDeadline = t | ||
| s.rDeadline.set(t) |
There was a problem hiding this comment.
We should try to prevent this from happening after closing the stream so we don't leak a timer. Not that much of an issue but is there some kind of state lock we can take?
There was a problem hiding this comment.
We could try the clLock and check if it has been closed.
There was a problem hiding this comment.
I used the clLock and return an error if the stream has been closed.
| defer s.deadlineLock.Unlock() | ||
| s.rDeadline = t | ||
| s.wDeadline = t | ||
| s.rDeadline.set(t) |
stream.go
Outdated
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-s.rDeadline.wait(): | ||
| return context.DeadlineExceeded |
There was a problem hiding this comment.
Should probably return a timeout error instead.
multiplex.go
Outdated
| type timeout struct{} | ||
|
|
||
| func (_ timeout) Error() string { | ||
| return "timeout" |
There was a problem hiding this comment.
"i/o deadline exceeded"? (or one for read/write?)
There was a problem hiding this comment.
yeah, that's better. will do.
stream.go
Outdated
| s.clLock.Lock() | ||
| defer s.clLock.Unlock() | ||
|
|
||
| if s.closedRemote && s.isClosed() { |
There was a problem hiding this comment.
this should probably be an or, will fix.
There was a problem hiding this comment.
Actually, this should probably be two cases. We want to be able to set a write deadline even when closed for reading and vice versa.
There was a problem hiding this comment.
It would be inconsistent to set one of the two deadlines I think.
We are creating tons of contexts when there are deadlines... one for every read/write.