sergot / http-useragent

Web user agent class for Perl 6.
MIT License
36 stars 39 forks source link

Reduce scope of SSL lock for better async support #235

Closed jjatria closed 3 years ago

jjatria commented 3 years ago

A lock was added in #193 (45fdcf359d) 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.

jjatria commented 3 years ago

The fundamental issue seems related to https://github.com/rakudo/rakudo/issues/1920

patrickbkr commented 3 years ago

Any reason this isn't merged yet?

ugexe commented 3 years ago

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