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.

Sign up for GitHub

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

stop the deadline timer in Stream.Read and Write #2519

Merged
merged 2 commits into from
May 5, 2020
Merged

stop the deadline timer in Stream.Read and Write #2519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93cee1e
stop the deadline timer in Stream.Read and Write
marten-seemann May 3, 2020
996dd42
move deadlineTimer declaration out of the Read loop
marten-seemann May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions receive_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ func (s *receiveStream) readImpl(p []byte) (bool /*stream completed */, int, err
}
if deadlineTimer == nil {
deadlineTimer = utils.NewTimer()
defer deadlineTimer.Stop()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@marten-seemann marten-seemann May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
deadlineTimer.Reset(deadline)
}
Expand Down
1 change: 1 addition & 0 deletions send_stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ func (s *sendStream) Write(p []byte) (int, error) {
}
if deadlineTimer == nil {
deadlineTimer = utils.NewTimer()
defer deadlineTimer.Stop()
}
deadlineTimer.Reset(deadline)
}
Expand Down