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

Stop the grace timer iff adding first handle (fix #99) #102

Merged
merged 2 commits into from
Feb 26, 2021
Merged

Stop the grace timer iff adding first handle (fix #99) #102

merged 2 commits into from
Feb 26, 2021

Conversation

StefanKarpinski
Copy link
Sponsor Member

No description provided.

@StefanKarpinski
Stop the grace timer iff adding first handle (fix #99)
841ac8f 
Using `multi.handle` to decide whether we need to stop the grace period
timer or not is confusing; the clearest criterion for whether we need to
start or stop that timer is whether the `multi.easies` vector is empty,
so this patch uses that criterion instead, thereby fixing #99.
@StefanKarpinski
Copy link
Sponsor Member Author

Alternative to #101. Thanks for the diagnosis, @maleadt — I think this fix is clearer and makes the start and stop logic for the grace timer more symmetrical and thus, imo, less confusing. The issue with stopping the timer whenever multi.handle is not NULL is that there may be a timer going that was set by another ongoing download and stopping that will cause different kinds of hangs. I believe this stops the timer if and only if there could be a grace timer going or when the multi handle has never been used, which is harmless since stopping a libuv timer that hasn't been started is a no-op (we could avoid doing that by also checking that multi.handle != C_NULL before stopping the timer).

@giordano
Copy link
Contributor

Just to make sure the test is reliable on GitHub Actions, I added Tim's test on top of master, without any of the fixes here or in #101, and with a timeout of 15 minutes, result: https://github.com/giordano/Downloads.jl/runs/1983343797. Interestingly enough, the only platform passing the tests without timing out is... 64-bit Windows.

@DilumAluthge
Copy link
Member

Does this PR close #99 and #101?

@fredrikekre
Copy link
Member

I can't repro #95 with this patch (with HTTP/2 turned on). Likely also fixes #94 but that one I have only been able to trigger in a running server and happens maybe once a day or something.

@fredrikekre fredrikekre merged commit 6bb8306 into master Feb 26, 2021
@fredrikekre fredrikekre deleted the sk/fix-bad-grace branch February 26, 2021 09:15
@fredrikekre fredrikekre mentioned this pull request Feb 26, 2021
Closed
@giordano giordano mentioned this pull request Feb 26, 2021
Open
@StefanKarpinski StefanKarpinski mentioned this pull request Feb 26, 2021
Closed
@jebej jebej mentioned this pull request Mar 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@StefanKarpinski @giordano @DilumAluthge @fredrikekre @maleadt