fix: add deadlines to write and read requests #81
Conversation
Prevents buildup in case of non-responding peers
dht_net.go
Outdated
| @@ -178,6 +179,7 @@ func (ms *messageSender) SendMessage(ctx context.Context, pmes *pb.Message) erro | |||
| } | |||
|
|
|||
| func (ms *messageSender) writeMessage(pmes *pb.Message) error { | |||
| ms.s.SetWriteDeadline(time.Now().Add(dhtMessageTimeout)) | |||
There was a problem hiding this comment.
Is the writeDeadline scoped per WriteMsg() call? Otherwise I think we might have to clear the deadline when we're finished with this writeMessage() call.
There was a problem hiding this comment.
All writes are pretended with it and clearing it would cause race where one write ends and another starts and the starting one gets it deadline cleared.
|
One tiny question, otherwise LGTM |
|
dont forget to reset the deadlines after the write succeeds |
|
I can't reset write deadline, it might race with another set of the deadline (in other goroutine). Will anything happen if write daedlines are set before every write? |
An extra syscall, probably (depending on the stream multiplexer implementation). |
That shouldnt happen. You shouldnt ever be writing to the same conn from different goroutines. |
|
@whyrusleeping Can one not write to the same stream from multiple goroutines? |
|
To answer my own question, yes (one shouldn't, at least). |
Thing is that we want those deadlines. I will reformulate my question: |
|
Yes. If the deadline expires, i believe that all future operations will
fail.
…On Wed, Aug 30, 2017, 6:58 PM Jakub Sztandera ***@***.***> wrote:
An extra syscall, probably (depending on the stream multiplexer
implementation).
Thing is that we want those deadlines. I will reformulate my question:
Will anything happen if we don't reset the deadlines but we set deadlines
before every write?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#81 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABL4HPS1M3y7Kw77n5KQk-8Nc6SJVHsTks5sdhNfgaJpZM4O1g1Z>
.
|
|
@whyrusleeping Only if you don't extend the deadline before the next write. This is actually how deadlines are supposed to be used as per the documentation: https://golang.org/pkg/net/#Conn |
|
Also another question: |
|
Per stream. Internally, streams set their deadlines before reading/writing and then unset them afterwards. |
|
(usually, each stream muxer has its own implementation) |
| @@ -63,6 +64,7 @@ func (dht *IpfsDHT) handleNewMessage(s inet.Stream) { | |||
| } | |||
|
|
|||
| // send out response msg | |||
| s.SetWriteDeadline(time.Now().Add(dhtMessageTimeout)) | |||
There was a problem hiding this comment.
Probably best to fix libp2p/go-mplex#7 first.
bigs
added
optimization
status/in-progress
|
do we want to rebase and merge this? |
|
We should probably fix libp2p/go-mplex#7 first. I just keep punting on it. |
dht_net.go
Outdated
| @@ -13,7 +13,7 @@ import ( | |||
| peer "github.com/libp2p/go-libp2p-peer" | |||
| ) | |||
|
|
|||
| var dhtReadMessageTimeout = time.Minute | |||
| var dhtMessageTimeout = 1 * time.Minute | |||
There was a problem hiding this comment.
I think this can be a const.
|
@Kubuxu do you still there's value in this? I've been messing around with this part of the code so I'm prepared to merge it if so. |
ghost
assigned
|
I merged master, I could have botched it, I'll look into it if this is still being pursued. |
|
Build failed due to gofmt error. |
|
@anacrolix I think few deadlines with WriteMsg are missing. Can you look over it? |
|
I want to clarify: Is the deadline use because Read and Write cannot take contexts, and it's preferable to using "ctxio" and having to terminate the connection to achieve cancellation? |
|
This would be unnecessary with #271. |
Prevents buildup in case of non-responding peers