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.

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

Remove uses of at-async in favor of at-spawn #1039

Merged
merged 4 commits into from
May 10, 2023
Merged

Remove uses of at-async in favor of at-spawn #1039

merged 4 commits into from
May 10, 2023

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented May 1, 2023

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2023

Codecov Report

Merging #1039 (d799ddb) into master (ab4d9b3) will decrease coverage by 0.35%.
The diff coverage is 67.24%.

❗ Current head d799ddb differs from pull request most recent head da7f237. Consider uploading reports for the commit da7f237 to get more accurate results

@@            Coverage Diff             @@
##           master    #1039      +/-   ##
==========================================
- Coverage   85.59%   85.24%   -0.35%     
==========================================
  Files          32       32              
  Lines        3020     3023       +3     
==========================================
- Hits         2585     2577       -8     
- Misses        435      446      +11     
Impacted Files Coverage Δ
src/Exceptions.jl 63.15% <0.00%> (-24.35%) ⬇️
src/HTTP.jl 67.05% <ø> (ø)
src/clientlayers/ConnectionRequest.jl 78.35% <50.00%> (-1.44%) ⬇️
src/Connections.jl 84.30% <60.00%> (-1.10%) ⬇️
src/clientlayers/ExceptionRequest.jl 83.33% <60.00%> (-7.58%) ⬇️
src/clientlayers/TimeoutRequest.jl 86.66% <77.77%> (-5.65%) ⬇️
src/clientlayers/StreamRequest.jl 95.50% <90.00%> (-1.05%) ⬇️
src/IOExtras.jl 77.77% <100.00%> (ø)
src/Streams.jl 96.21% <100.00%> (+0.54%) ⬆️

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

@quinnj quinnj requested a review from nickrobinson251 May 1, 2023 12:59
@quinnj quinnj changed the title Add threadid to connection key to try and pin connections to specific threads Remove uses of at-async in favor of at-spawn May 1, 2023
src/clientlayers/ConnectionRequest.jl Outdated Show resolved Hide resolved
src/clientlayers/ConnectionRequest.jl Outdated Show resolved Hide resolved
src/Exceptions.jl Outdated Show resolved Hide resolved
@quinnj
Copy link
Member Author

quinnj commented May 6, 2023

Tests currently failing until JuliaServices/ConcurrentUtilities.jl#22 is merged and tagged.

…on" cooperating

tasks to be sticky to the thread that is running them or the parent's task's thread.
Copy link
Collaborator

@Drvi Drvi left a comment

Choose a reason for hiding this comment

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

Looks good to me:) I think the @async -> @spawn changes are not the most complicated, it's more the switch to OpenSSL and the changes to exception handling which are more difficult to access. But since these are all based on your experiments with large workload, I believe they'll make nice improvements in terms of performance and log deduplication.

test/client.jl Outdated Show resolved Hide resolved
@@ -10,14 +10,15 @@ using ..IOExtras, ..Messages, ..Exceptions
Throw a `StatusError` if the request returns an error response status.
"""
function exceptionlayer(handler)
return function exceptions(stream; status_exception::Bool=true, logerrors::Bool=false, kw...)
res = handler(stream; logerrors=logerrors, kw...)
return function exceptions(stream; status_exception::Bool=true, timedout=nothing, logerrors::Bool=false, logtag=nothing, kw...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, timedout kwarg is strictly internal thing, the user shouldn't provide their own TimedOut struct to a HTTP.get? If that is the case, we might want to call it _timedout just to make it clearer that this is an internal thing.

src/clientlayers/ConnectionRequest.jl Outdated Show resolved Hide resolved
@quinnj quinnj merged commit b64beb7 into master May 10, 2023
@quinnj quinnj deleted the jq-threadsafe branch May 10, 2023 21:57
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