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

Jump to bottom

Regression in 2.3.2: Long download closed after StreamTimeout even if data still moving #140

Closed
notsure2 opened this issue Dec 6, 2020 · 1 comment · Fixed by #141
Closed

Regression in 2.3.2: Long download closed after StreamTimeout even if data still moving #140

notsure2 opened this issue Dec 6, 2020 · 1 comment · Fixed by #141

Comments

@notsure2
Copy link
Contributor

notsure2 commented Dec 6, 2020 •
edited
Loading

Hello @cbeuw
I think in the latest pull request, we accidentally made regression. If you set StreamTimeout to be a low value (eg: 5 seconds) and start a long download such that the download takes more than StreamTimeout seconds, the session will be forcefully closed even if there was data being downloaded. I will investigate.

The session needs to be kept alive if there is any traffic passing in it.
@notsure2
Copy link
Contributor Author

notsure2 commented Dec 6, 2020 •
edited
Loading

Ok upon further investigation, this is not a regression. The bug was there from the start (not refreshing the read deadline when there's activity) but it was working by luck because streamBufferPipe was calling timer.AfterFunc to schedule a wake up just a very short time before the deadline actually passed every loop, so the StreamTimeout was actually malfunctioning and not being applied correctly.

Cloak/internal/multiplex/streamBufferedPipe.go

Lines 64 to 92 in f1c6567

for {
if p.closed && p.buf.Len() == 0 {
return 0, io.EOF
}
if !p.rDeadline.IsZero() {
d := time.Until(p.rDeadline)
if d <= 0 {
return 0, ErrTimeout
}
if p.wtTimeout == 0 {
// if there hasn't been a scheduled broadcast
time.AfterFunc(d, p.rwCond.Broadcast)
}
}
if p.wtTimeout != 0 {
p.rDeadline = time.Now().Add(p.wtTimeout)
time.AfterFunc(p.wtTimeout, p.rwCond.Broadcast)
}
if p.buf.Len() > 0 {
written, er := p.buf.WriteTo(w)
n += written
if er != nil {
p.rwCond.Broadcast()
return n, er
}
p.rwCond.Broadcast()
} else {
p.rwCond.Wait()
}

In 2.3.2 the StreamTimeout is now being enforced correctly, however, there is a new behavior that is required, which is having any activity on the other side reset the timeout (to handle the case of HTTP download, the browser sends a short data then sends nothing, and the rest of the time is spent downloading).

Actually, even in 2.3.2 there is still a bug, if a browser uploads data continuously for the duration of the StreamTimeout, now the connection will be forcefully closed also because the deadline is not reset every loop (only when the buffer runs out).

@notsure2 notsure2 mentioned this issue Dec 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix regression: termination of long downloads after StreamTimeout seconds notsure2/Cloak
1 participant
@notsure2