Re: [PATCH] Get rid of Kgio

[Eric Wong <e@80x24.org>]

Jean Boussier <jean.boussier@shopify.com> wrote:
> As discussed kgio is no longer absolutely necessary.
> 
> We can use Ruby 2+ non blocking IO capabilities instead.

I think there was a misunderstanding, here.  I meant kgio wasn't
necessary for new process management code.  Notes inline...

> diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
> index 21f2a05..98cd119 100644
> --- a/lib/unicorn/http_server.rb
> +++ b/lib/unicorn/http_server.rb

> @@ -733,9 +742,15 @@ def worker_loop(worker)
>        reopen = reopen_worker_logs(worker.nr) if reopen
>        worker.tick = time_now.to_i
>        while sock = ready.shift
> -        # Unicorn::Worker#kgio_tryaccept is not like accept(2) at all,
> +        # Unicorn::Worker#accept_nonblock is not like accept(2) at all,
>          # but that will return false
> -        if client = sock.kgio_tryaccept
> +        client = begin
> +          sock.accept_nonblock
> +        rescue Errno::EAGAIN
> +          false
> +        end
> +

Exception garbage in hotter code paths like accept_nonblock was
the reason kgio exists, especially for trivial Rack apps I tend
to write.  Ruby 2.3+ added `exception: false' to accept_nonblock.

It's less bad for Linux nowadays with EPOLLEXCLUSIVE, but that's
not portable.

> diff --git a/lib/unicorn/stream_input.rb b/lib/unicorn/stream_input.rb
> index 41d28a0..3241ff4 100644
> --- a/lib/unicorn/stream_input.rb
> +++ b/lib/unicorn/stream_input.rb

> @@ -72,8 +77,7 @@ def read(length = nil, rv = '')
>    # Returns nil if called at the end of file.
>    # This takes zero arguments for strict Rack::Lint compatibility,
>    # unlike IO#gets.
> -  def gets
> -    sep = $/
> +  def gets(sep = $/)
>      if sep.nil?
>        read_all(rv = '')
>        return rv.empty? ? nil : rv

This change to #gets is unrelated and should've belonged in a
separate commit.  But the change itself seems wrong, too, given
what Rack::Lint enforces.  Note the comment above the method.
I also just checked current Rack::Lint::InputWrapper#gets,
and it still forbids args.

Thanks.