Skip to content

wait on the correct object, instead of calling sleep#881

Closed
vtjnash wants to merge 2 commits intoJuliaWeb:masterfrom
vtjnash:jn/sleepy-cleanup
Closed

wait on the correct object, instead of calling sleep#881
vtjnash wants to merge 2 commits intoJuliaWeb:masterfrom
vtjnash:jn/sleepy-cleanup

Conversation

@vtjnash
Copy link
Copy Markdown
Member

@vtjnash vtjnash commented Jul 15, 2022

Using sleep usually indicates a design issue with the code, and
introduced a few minor data races into this code. Provides various
opportunities for code cleanup also.

Seemed to be needed along with JuliaLang/MbedTLS.jl#243, though I can't remember why.

close(s.listener)
# first pass to mark or request connections to close
for c in s.connections
for c in collect(s.connections)
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.

why collect here?

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.

close is a yield point and you are missing a lock here. The collect helps minimize the data race window

- Using sleep usually indicates a design issue with the code, and
introduced a few minor data races into this code.
- Provides various opportunities for code cleanup.
- Provides some opportunities for fixing some of the error handling.
@vtjnash vtjnash force-pushed the jn/sleepy-cleanup branch from ef8eff7 to c8e0792 Compare July 18, 2022 17:35
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 18, 2022

Codecov Report

Merging #881 (f724d55) into master (84808c8) will increase coverage by 0.55%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master     #881      +/-   ##
==========================================
+ Coverage   79.94%   80.50%   +0.55%     
==========================================
  Files          36       36              
  Lines        2872     3072     +200     
==========================================
+ Hits         2296     2473     +177     
- Misses        576      599      +23     
Impacted Files Coverage Δ
src/Exceptions.jl 87.50% <ø> (ø)
src/ConnectionPool.jl 89.56% <81.81%> (+0.12%) ⬆️
src/Servers.jl 77.17% <91.66%> (-1.59%) ⬇️
src/HTTP.jl 75.96% <0.00%> (-7.09%) ⬇️
src/connectionpools.jl 72.72% <0.00%> (-3.57%) ⬇️
src/IOExtras.jl 77.14% <0.00%> (-2.86%) ⬇️
src/clientlayers/RetryRequest.jl 73.91% <0.00%> (-2.84%) ⬇️
src/clientlayers/StreamRequest.jl 97.14% <0.00%> (-1.43%) ⬇️
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@quinnj quinnj closed this Aug 17, 2022
@quinnj quinnj reopened this Aug 17, 2022
@quinnj
Copy link
Copy Markdown
Member

quinnj commented Nov 3, 2022

I think this has been integrated piecemeal in other PRs.

@quinnj quinnj closed this Nov 3, 2022
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.

3 participants