Skip to content

Revert 156 revert 151 sk/no libuv sushi#157

Merged
StefanKarpinski merged 2 commits into
masterfrom
revert-156-revert-151-sk/no-libuv-sushi
Nov 1, 2021
Merged

Revert 156 revert 151 sk/no libuv sushi#157
StefanKarpinski merged 2 commits into
masterfrom
revert-156-revert-151-sk/no-libuv-sushi

Conversation

@vtjnash

@vtjnash vtjnash commented Oct 29, 2021

Copy link
Copy Markdown
Member

Just editing on Github, so I might have made some typos. There was lock-acquires missing in the @async / Timer blocks of code.

@codecov

codecov Bot commented Oct 29, 2021

Copy link
Copy Markdown

Codecov Report

Merging #157 (43a3484) into master (c91876a) will decrease coverage by 1.92%.
The diff coverage is 92.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
- Coverage   93.02%   91.10%   -1.93%     
==========================================
  Files           5        5              
  Lines         516      472      -44     
==========================================
- Hits          480      430      -50     
- Misses         36       42       +6     
Impacted Files Coverage Δ
src/Curl/Multi.jl 93.33% <91.17%> (-2.73%) ⬇️
src/Curl/Curl.jl 100.00% <100.00%> (ø)
src/Curl/utils.jl 66.66% <100.00%> (-31.01%) ⬇️

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 c91876a...43a3484. Read the comment docs.

Comment thread src/Curl/Multi.jl
isopen(multi.timer) && done!(multi)
multi.timer = Timer(multi.grace/1000) do
lock(multi.lock) do
isopen(multi.timer) && done!(multi)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the isopen check necessary? You had mentioned that the isopen test was already done by the callback form of this. Or is it necessary because it might have been closed between when the timer fires and the done! call?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it is necessary both because it might have closed while we were waiting for the lock, and because the callback form does not check this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I misunderstood then.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I might have said it wrong too

@StefanKarpinski
StefanKarpinski force-pushed the revert-156-revert-151-sk/no-libuv-sushi branch from e76ef88 to 43a3484 Compare November 1, 2021 17:56
@StefanKarpinski
StefanKarpinski merged commit f856630 into master Nov 1, 2021
@StefanKarpinski
StefanKarpinski deleted the revert-156-revert-151-sk/no-libuv-sushi branch November 1, 2021 18:05
Comment thread src/Curl/Multi.jl
done!(multi)
elseif 0 < multi.grace < typemax(multi.grace)
uv_timer_start(multi.timer, cleanup_cb, multi.grace, 0)
multi.timer = Timer(multi.grace/1000) do

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Missing argument do t

Comment thread src/Curl/Multi.jl
uv_timer_start(multi.timer, cleanup_cb, multi.grace, 0)
multi.timer = Timer(multi.grace/1000) do
lock(multi.lock) do
isopen(multi.timer) && done!(multi)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This needs to check t, not multi.timer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is t?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants