Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle waking up a closed selector in Reactor#add #3005

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

JuanitoFatas
Copy link
Contributor

@JuanitoFatas JuanitoFatas commented Oct 28, 2022

This Pull Request rescues IOError when we try to wake up a closed selector.

I noticed there are IOErrors from this code path (see below). It may be auto-scaling scale up/down an instance. It does not seems to relate to during deployment / restart. I am not really sure. It only happens 4-5 times for the past 30 days in an app (with reasonable, constant traffic) I am working on.

vendor/bundle/ruby/2.7.0/gems/puma-5.6.4/lib/puma/reactor.rb:51:in `wakeup': selector is closed (IOError)
    from vendor/bundle/ruby/2.7.0/gems/puma-5.6.4/lib/puma/reactor.rb:51:in `add'
    from vendor/bundle/ruby/2.7.0/gems/puma-5.6.4/lib/puma/server.rb:470:in `process_client'
    from vendor/bundle/ruby/2.7.0/gems/puma-5.6.4/lib/puma/thread_pool.rb:147:in `block in spawn_thread'

Seems like similar to #shutdown method, we should handle waking up a closed selector.

I am not sure how to add a test for this.

If needs, please let me know how could I add some instrument to the app I’m working on, so I could help report more details for helping with improving this area. Thanks!

@MSP-Greg
Copy link
Member

MSP-Greg commented Oct 28, 2022

@JuanitoFatas Thanks for looking at this.

I've also seen very intermittent errors in CI, and it seems to be a problem related to how 'lost/dropped' connections are handled in the Reactor. Does that sound like it's similar to your issue?

Haven't had time to look at it, as I wanted to look at how a 'lost/dropped' connection is removed...

EDIT: Forget the above, the error is raised in NIO code, and 'selector is closed' is the message.

@MSP-Greg
Copy link
Member

Just a quick look, should this be:

    # Returns false if the reactor is already shut down.
    def add(client)
      @input << client
      @selector.wakeup
      true
    rescue ClosedQueueError, IOError
      false
    end

@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor CI / Testing labels Nov 3, 2022
Co-Authored-By: MSP-Greg <MSP-Greg@users.noreply.github.com>
@JuanitoFatas JuanitoFatas marked this pull request as ready for review November 3, 2022 11:19
@JuanitoFatas JuanitoFatas reopened this Nov 3, 2022
@nateberkopec
Copy link
Member

Test fails are not related, also happening on main.

@nateberkopec nateberkopec merged commit 70600bf into puma:master Nov 4, 2022
@JuanitoFatas JuanitoFatas deleted the reactor-closed-selector branch November 8, 2022 12:31
@MSP-Greg MSP-Greg added bug and removed waiting-for-changes Waiting on changes from the requestor CI / Testing labels Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants