search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.47k stars 1.62k forks source link

Cannot perform concurrent HTTPS GET requests from a single client session #12412

Open HertzDevil opened 2 years ago

HertzDevil commented 2 years ago

The following code seems to always crash:

require "http/client"

client1 = HTTP::Client.new("example.com", tls: true)
client2 = client1
ch1 = Channel(Int32).new
ch2 = Channel(Int32).new

puts LibSSL::OPENSSL_VERSION

5.times do |i|
  spawn do
    response = client1.get "/"
    ch1.send response.body.size
  end

  spawn do
    response = client2.get "/"
    ch2.send response.body.size
  end

  puts i
  puts "size = #{ch1.receive}"
  puts "size = #{ch2.receive}"
end
3.0.5
0
size = 1256
size = 1256
1
size = 1256
Invalid memory access (signal 11) at address 0x0
[0x55a32ed51f06] *Exception::CallStack::print_backtrace:Nil +118 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ed39bda] ~procProc(Int32, Pointer(LibC::SiginfoT), Pointer(Void), Nil) +330 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x7f7b3003daf0] ?? +140167063263984 in /lib/x86_64-linux-gnu/libc.so.6
[0x7f7b3009693c] ?? +140167063628092 in /lib/x86_64-linux-gnu/libc.so.6
[0x7f7b30097275] malloc +149 in /lib/x86_64-linux-gnu/libc.so.6
[0x7f7b30a55113] ?? +140167073845523 in /usr/lib/x86_64-linux-gnu/libssl.so.3
[0x7f7b30a524f5] ?? +140167073834229 in /usr/lib/x86_64-linux-gnu/libssl.so.3
[0x7f7b30a55e78] ?? +140167073848952 in /usr/lib/x86_64-linux-gnu/libssl.so.3
[0x7f7b30a543f0] ?? +140167073842160 in /usr/lib/x86_64-linux-gnu/libssl.so.3
[0x7f7b30a296ae] ?? +140167073666734 in /usr/lib/x86_64-linux-gnu/libssl.so.3
[0x7f7b30a35283] SSL_read +35 in /usr/lib/x86_64-linux-gnu/libssl.so.3
[0x55a32ee26eed] *OpenSSL::SSL::Socket+ +109 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee26d5e] *OpenSSL::SSL::Socket+ +78 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee26c86] *OpenSSL::SSL::Socket+ +54 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee198b2] *IO+ +1186 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee1d08a] *IO+ +170 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee78577] *HTTP::Client::Response::from_io?<IO+, Bool, Bool>:(HTTP::Client::Response | Nil) +103 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee60360] *HTTP::Client#exec_internal_single<HTTP::Request>:(HTTP::Client::Response | Nil) +80 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee600dd] *HTTP::Client#exec_internal<HTTP::Request>:HTTP::Client::Response +29 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee5fdd7] *HTTP::Client#exec<HTTP::Request>:HTTP::Client::Response +71 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee5fd76] *HTTP::Client#exec<String, String, Nil, Nil>:HTTP::Client::Response +22 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ee5fd57] *HTTP::Client#get<String>:HTTP::Client::Response +39 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ed3e661] ~procProc(Nil) +33 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ed9c0f9] *Fiber#run:(IO::FileDescriptor | Nil) +185 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x55a32ed396f6] ~proc2Proc(Fiber, (IO::FileDescriptor | Nil)) +6 in /home/quinton/.cache/crystal/crystal-run-test.tmp
[0x0] ???

The same works for non-HTTPS connections and also if two different HTTP::Clients sharing the same TLS context are used:

tls_cxt = OpenSSL::SSL::Context::Client.new
client1 = HTTP::Client.new("example.com", tls: tls_cxt)
client2 = HTTP::Client.new("example.com", tls: tls_cxt)
z64 commented 2 years ago

Regardles of HTTPS, it is not safe to do this with an instance of HTTP::Client and ensure correctness.

HTTP::Client is not "fiber-safe" as-designed, as you have found out - but besides TLS negotiation problems, one fiber may receive the response of a totally different request started by another, or other permutations. So this is probably never a good idea.

Anyways - there are probably two ways to fix the segfault off the top of my head:

  1. Right now, HTTP::Client.new will lazy-connect. Meaning HTTP::Client.new doesn't do anything until you call .get or some other action. It could be changed so that it connects right away, ensuring only one fiber does the setup.
  2. Some kind of mutex scheme around TLS handshake, which doesn't sound great.

Personally I would like to see (1). I think this is fine. I patch HTTP::Client to do this anyways because I set up connection pools.

asterite commented 2 years ago

I think we should eventually change HTTP::Client so that it's fiber-safe.

mloughran commented 2 years ago

In the meantime, it would be great if the docs were be updated to state that HTTP::Client is not fiber-safe. I made the same assumption (with the aim of reusing a TCP connection), and was similarly bitten.

straight-shoota commented 2 years ago

Meta issue on refactoring HTTP::Client: https://github.com/crystal-lang/crystal/issues/6011