ohler55 / oj

Optimized JSON
http://www.ohler.com/oj
MIT License
3.14k stars 251 forks source link

to_stream and nonblocking sockets #723

Open carlhoerberg opened 2 years ago

carlhoerberg commented 2 years ago

Since Ruby 3.0 sockets are by default nonblocking, which causes issues for Oj.to_stream here

require "oj"
require "socket"
require 'io/nonblock'
pid = spawn("nc -d 0.1 -l 5000", out: "/dev/null")
at_exit { Process.kill 9, pid }
sleep 0.1
s = Socket.tcp("localhost", 5000)
# s.nonblock = false
1_000_000.times do
  Oj.to_stream(s, { a: 1 })
end

output:

n.rb:10:in `to_stream': Write failed. [11:Resource temporarily unavailable] (IOError)
        from n.rb:10:in `block in <main>'
        from n.rb:9:in `times'
        from n.rb:9:in `<main>'

set s.nonblock = false and it works as expected..

Not sure if something should be done in Oj, but could be good for ppl to be aware.. It's not very ergonomic to catch the generic IOError and then trying to figure out if it's a EAGAIN error and manually do IO.select.

carlhoerberg commented 2 years ago

Maybe use pwritev2 with the RWF_SYNC flag, but that's Linux only.. https://man7.org/linux/man-pages/man2/pwritev2.2.html

ohler55 commented 2 years ago

I'm hesitant to make that the default but it would be a nice option for Oj.to_stream. What do you think?

ohler55 commented 2 years ago

Looking at this again. I think I misunderstood the desired changes. I don't think Oj should set the stream to non blocking but I do think Oj could handle EAGAIN errors along with some timeout value. I'm looking into that now.

ohler55 commented 2 years ago

Please try branch "non-blocking". Test added as well although I found the issue is exposed more readily with a large single write instead of many smaller ones and the test reflects that.

ohler55 commented 2 years ago

I believe the non-blocking branch fixes the issue. I plan to release tomorrow barring feedback from you indicating it is not fixed.

ohler55 commented 2 years ago

Okay to close?

carlhoerberg commented 2 years ago

is the patch included in oj 3.13.11? The example in the original issue still fails with it