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

Reduce scope of SSL lock for better async support #235

Merged
merged 1 commit into from
Nov 7, 2020
Merged

Reduce scope of SSL lock for better async support #235

merged 1 commit into from
Nov 7, 2020

Conversation

jjatria
Copy link
Contributor

@jjatria jjatria commented Oct 14, 2020

A lock was added in #193 (45fdcf3) to fix an issue resulting in a segmentation fault being triggered when creating IO::Socket::SSL objects in an async process. At the time, the reason for the error was assumed to be in IO::Socket::SSL or in one of its underlying
dependencies.

The lock added protected the entire block where IO::Socket::SSL was conditionally loaded and a new socket was constructed. While this solved the problem, it meant that while using HTTPS, HTTP::UserAgent was fundamentally incompatible with async environments.

However, the issue does not seem to be related to IO::Socket::SSL but to the conditional loading logic, and reducing the scope of the lock not only continues to protect against the error, but results in much better support for async requests:

# async.raku
use HTTP::UserAgent;
my $ua = HTTP::UserAgent.new;
await ( ^10 ).map: -> $i {
    start { note "#$i: { .status-line }" with $ua.get: 'https://httpbin.org/status/200' }
}

Before the change (note that requests are resolved sequentially):

$ time raku -I. ~/async.raku
#0: 200 OK
#1: 200 OK
#2: 200 OK
#3: 200 OK
#4: 200 OK
#5: 200 OK
#6: 200 OK
#7: 200 OK
#8: 200 OK
#9: 200 OK

real    0m3.583s
user    0m1.548s
sys     0m0.114s

After the change:

$ time raku -I. ~/async.raku
#3: 200 OK
#4: 200 OK
#1: 200 OK
#2: 200 OK
#6: 200 OK
#0: 200 OK
#5: 200 OK
#7: 200 OK
#9: 200 OK
#8: 200 OK

real    0m1.286s
user    0m1.430s
sys     0m0.090s

You can actually easily reproduce the segmentation fault without IO::Socket::SSL or any related classes:

$ raku -e 'await ( ^10 ).map: { start { require ::("JSON::Fast") }  }'
Segmentation fault (core dumped)

I came across this while working on the equivalent change for HTTP::Tiny and I thought this project could use this as well.

A lock was added in #193 (45fdcf3) to fix an issue resulting in
a segmentation fault being triggered when creating IO::Socket::SSL
objects in an async process. At the time, the reason for the error
was assumed to be in IO::Socket::SSL or in one of its underlying
dependencies.

The lock added protected the entire block where IO::Socket::SSL was
conditionally loaded and a new socket was constructed. While this
solved the problem, it meant that while using HTTPS, HTTP::UserAgent
was fundamentally incompatible with async environments.

However, the issue does not seem to be related to IO::Socket::SSL
but to the conditional loading logic, and reducing the scope of the
lock not only continues to protect against the error, but results in
much better support for async requests.
@jjatria
Copy link
Contributor Author

jjatria commented Oct 14, 2020

The fundamental issue seems related to rakudo/rakudo#1920

@patrickbkr
Copy link

Any reason this isn't merged yet?

@ugexe ugexe merged commit 218aed4 into sergot:master Nov 7, 2020
@ugexe
Copy link
Collaborator

ugexe commented Nov 7, 2020

Might be worth investigating using https://docs.raku.org/type/Lock::Async instead of Lock

@jjatria jjatria deleted the async-lock branch November 12, 2020 14:05
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