Defer checking if socket is closed

[calvinxiao]

Avoid making getsockopt system call for every request

Description

In function handle_request, we can check if the socket is closed when rescuing IOError, saving one getsockopt system call for every request.

In the current implementation, the socket may be closed by the client after we check the socket status using closed_socket? and before finishing writing the data.

Writing to a closed socket will raise IOError. I am not sure which of the following Error I should use, my commit uses "Socket timeout writing data"

raise ConnectionError, "Connection error detected during write"

raise ConnectionError, "Socket timeout writing data"

Analysis detail:

1. Run hello.ru benchmark for 30 seconds,
2. While running benchmark, run sudo perf stat -e 'syscalls:*' -p $(pidof bundle) sleep 10, this will use perf to collect all syscalls made by puma process for 10 seconds

perf stat result:

 Performance counter stats for process id '1732':

             42009      syscalls:sys_enter_getpeername
             42009      syscalls:sys_exit_getpeername
             42009      syscalls:sys_enter_recvfrom
             42010      syscalls:sys_exit_recvfrom
             84019      syscalls:sys_enter_setsockopt
             84021      syscalls:sys_exit_setsockopt
             42010      syscalls:sys_enter_getsockopt
             42010      syscalls:sys_exit_getsockopt
              8215      syscalls:sys_enter_epoll_ctl
              8215      syscalls:sys_exit_epoll_ctl
              8156      syscalls:sys_enter_epoll_wait
              8156      syscalls:sys_exit_epoll_wait
             37811      syscalls:sys_enter_select
             37811      syscalls:sys_exit_select
                11      syscalls:sys_enter_ppoll
                11      syscalls:sys_exit_ppoll
              6235      syscalls:sys_enter_read
              6235      syscalls:sys_exit_read
             88227      syscalls:sys_enter_write
             88226      syscalls:sys_exit_write
                 1      syscalls:sys_enter_madvise
                 1      syscalls:sys_exit_madvise
                21      syscalls:sys_enter_mprotect
                21      syscalls:sys_exit_mprotect
            274663      syscalls:sys_enter_futex
            274659      syscalls:sys_exit_futex
                 1      syscalls:sys_enter_sched_yield
                 1      syscalls:sys_exit_sched_yield

Benchmark RPS is around 4k to 5k, here is my basic understanding of what each syscall is used for:

• getpeername, for getting the peer IP address
• recvfrom, ?
• setsockopt, cork_socket and uncork_socket, this will be removed in Improve client response code, chunked bodies #2595
• getsockopt, check socket status to see if the socket is closed
• epoll_*, epoll functions used in nio4r
• select, IO.select will get the socket that is ready for reading or writing
• ppoll, probably some blocking IO functions
• read and write, for reading request and writing response data
• madvise and mprotect, GC stuffs
• futex, puma uses threads, mutex and conditional variable would cause lot's of futex usage, looking forward to getting rid of them(ALL) in Support for non-blocking fibers #2601
• sched_yield, process management, https://linux.die.net/man/2/sched_yield

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[calvinxiao]

I didn't find any performance improvement, would be nice to save one system call though.

Just find a slide explaining ruby and system calls, https://speakerdeck.com/franckverrot/rubykaigi-2016-hijacking-syscalls-with-ruby?slide=12

Also, getsockopt is very fast:

Warming up --------------------------------------
          getsockopt   119.930k i/100ms
Calculating -------------------------------------
          getsockopt      1.202M (± 3.1%) i/s -      6.116M in   5.094363s

[calvinxiao]

For the recvfrom syscall, it's used in read_nonblock to check IO::WaitReadable etc.

• https://github.com/ruby/ruby/blob/78f188524f551c97b1a7a44ae13514729f1a21c7/ext/socket/init.c#L341
• https://linux.die.net/man/2/recvfrom

MSG_DONTWAIT (since Linux 2.2)
Enables nonblocking operation; if the operation would block, the call fails with the error EAGAIN or EWOULDBLOCK (this can also be enabled using the O_NONBLOCK flag with the F_SETFL fcntl(2)).

[MSP-Greg]

@calvinxiao

Isn't return false if closed_socket?(io) used to prevent calling the app if the client/socket is closed?

Moving into an else added to if @early_hints might be a better place for it?

[calvinxiao]

@MSP-Greg

Since closed_socket?(io) is very fast, calling the app when the socket is closed will cost much more.

I just tested that client can send data then close, the server is able to read the data but cannot write, we should close this PR in case of some DDOS attack.

Learned a lot when perfing puma. 😎

[MSP-Greg]

Learned a lot when perfing puma.

Me also. While working on #2595, I was reviewing Rails & Rack code, more than I've ever done before. I started with every optimization I could see, tried all the Rack responses I could think up, sat on the code for a bit, then reverted the 'micro' optimizations that made the code more complex but yielded very small improvements.

we should close this PR in case of some DDOS attack.

Probably right... Thanks, Greg