Reduce scope of SSL lock for better async support #235
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Before the change (note that requests are resolved sequentially):
After the change:
You can actually easily reproduce the segmentation fault without IO::Socket::SSL or any related classes:
I came across this while working on the equivalent change for HTTP::Tiny and I thought this project could use this as well.