stop the deadline timer in Stream.Read and Write

[marten-seemann]

When a deadline is set on a stream, the time.Time is saved in the deadline member variable.
When Read or Write are called, and deadline is set, we start a timer in order to return these calls on timer expiration. In the common case, deadlines don't expire, and Read and Write calls return before expiration of the timer. We need to make sure to stop this timer, otherwise it will leak.

This is the more probable reason for the performance issue reported in ipfs/kubo#7263, as every single Read and Write call that returns before the deadline leaks a new timer. This will accumulate a lot more timers than the timer leak that happened when closing a session, or when we received a duplicate Initial, which was fixed in #2516.

cc @Stebalien

[Stebalien]

• This will defer until the function returns. Are you sure this won't happen multiple times.
• Alternatively, why is the deadline timer not defined outside the outer loop?
• More generally, why are we looping in the first place instead of reading one frame and returning?

[marten-seemann]

This will defer until the function returns. Are you sure this won't happen multiple times.

Yes, deadlineTimer is only set if deadlineTimer == nil, so it can only happen once.

Alternatively, why is the deadline timer not defined outside the outer loop?

If no deadline is set, there's no need to start a timer. Note that the user might set a deadline concurrently with a Read call.

More generally, why are we looping in the first place instead of reading one frame and returning?

That's due to the way frames are processed: every time a STREAM frame is received on that stream, the Read loop wakes up and tries to read data. STREAM frames might arrive out of order though, so we need to repeat this until a STREAM frame at the current read offset is received.

More generally speaking, I wish there was a way to concurrently use a timer. Then I could just set the timer in SetDeadline and use it in Read. This whole dance of saving the deadline as a timestamp and then creating and destroying the timer in Read is only necessary because Go messed up the timer API.

[Stebalien]

Ah, got it.

[Stebalien]

But are you sure we can't loop on line 107?

[marten-seemann]

But are you sure we can't loop on line 107?

I don't understand the question. The loop is on line 107.

[Stebalien]

Basically, either:

1. The loop on line 107 is useless.
2. We can reach this code multiple times (creating multiple deadline timers).

My suggestion was moving the deadlineTimer variable declaration above the outer loop because it's currently inside the outer loop.

[marten-seemann]

The use case is the following:

1. The user calls Read. The call blocks, since there's no data to read and we're waiting for the STREAM frame at the right offset to arrive.
2. The user then calls SetReadDeadline.
3. After this, the STREAM frame is received (or the deadline expires).

My suggestion was moving the deadlineTimer variable declaration above the outer loop because it's currently inside the outer loop.

I guess you could declare it as a pointer to a timer (so we don't actually have to start a timer when no deadline is set), and then have a defer that stops the timer if it's != nil.

[Stebalien]

It sounds like you're talking about the inner loop. I'm talking about the outer loop.

I guess you could declare it as a pointer to a timer (so we don't actually have to start a timer when no deadline is set), and then have a defer that stops the timer if it's != nil.

That's what you're already doing, isn't it? I'm literally just suggesting that the declaration on line 115 be moved to line 106. If you do that, looping at line 107 won't re-declare a new deadlineTimer variable, causing you to allocate a new timer on line 134.

[marten-seemann]

That makes sense. And it's consistent with what we do in the sending half of the stream. Thank you!

[codecov-io]

Codecov Report

Merging #2519 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2519   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files         122      122           
  Lines        9536     9538    +2     
=======================================
+ Hits         8214     8216    +2     
  Misses        984      984           
  Partials      338      338           

Impacted Files	Coverage Δ
receive_stream.go	96.43% <100.00%> (+0.02%)	⬆️
send_stream.go	92.99% <100.00%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0b6d4e...996dd42. Read the comment docs.

[Stebalien]

LGTM!