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

Improvements to out_of_band hook #2234

Merged
merged 4 commits into from Apr 30, 2020
Merged

Conversation

@wjordan
Copy link
Contributor

wjordan commented Apr 24, 2020

Description

Followup to #2230 (extending the idea to move the OOB hook from Server into ThreadPool), addressing the two extra edge cases described at #2230 (comment).

Also:

  • Refactor existing out-of-band tests from the big TestPumaServer class into its own TestOutOfBandServer class
  • Add extra tests covering the new edge-cases handled
  • A bit more cleanup/documentation on the test suite (added high-level comments describing the behavior expected by each test)

(Turns out there are a ton of complicated little edge-cases to consider around what seemed like such a simple feature!)

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added 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.
This moves the hook to be executed outside of busy loop
considering the thread to be truly idle at a time of execution
of the hook
@wjordan wjordan force-pushed the wjordan:improve-oob-hook branch from 3aa150f to 58014f4 Apr 24, 2020
end
end
private :accept_socket

This comment has been minimized.

Copy link
@wjordan

wjordan Apr 24, 2020

Author Contributor

Extracted this logic into a private method both to simplify the accept-loop, but more importantly to allow the to be stubbed in the test_blocks_new_connection unit-test.

This comment has been minimized.

Copy link
@wjordan

wjordan Apr 25, 2020

Author Contributor

hmm, the original ECONNABORTED rescue clause had io.close not sock.close, I should change this back to the original behavior.

That said, the line is ancient, dating back to 2006 (ab3c808) (!) and never covered by any tests, so it's very likely this is 'superstitious' code that should probably be changed/removed separately anyway.

In my reading of the current code, ECONNABORTED would only ever be thrown by accept_nonblock (accept syscall) if the listener socket is closed accepts a connection that was closed while waiting on the listen queue, but in that case io would be nil causing io.close to always fail..

This comment has been minimized.

Copy link
@wjordan

wjordan Apr 26, 2020

Author Contributor

I managed to figure out how to stub the accept call without needing to extract the method, so I'll move this refactoring/cleanup to a separate PR.

io.close
rescue
Thread.current.purge_interrupt_queue if Thread.current.respond_to? :purge_interrupt_queue
pool.wait_until_not_full

This comment has been minimized.

Copy link
@wjordan

wjordan Apr 24, 2020

Author Contributor

Moving the call to wait_until_not_full before accept_socket prevents connections from being accepted until out_of_band hooks are completed.

(I'm not 100% sure this change won't have any other side-effects, but the test suite seems to think it's okay...)

An alternative would be to just add an extra pool.with_mutex {} line above accept_socket.

@@ -224,8 +224,11 @@ def run(background=true)
@reactor.add client
end
end

process_now

This comment has been minimized.

Copy link
@wjordan

wjordan Apr 24, 2020

Author Contributor

This return value of the ThreadPool block (process_now is true when a complete request was processed by the block) is used to determine if an OOB hook should be scheduled to fire once the thread is eventually idle.

@wjordan wjordan force-pushed the wjordan:improve-oob-hook branch from 58014f4 to bad2be5 Apr 26, 2020
wjordan added 2 commits Apr 24, 2020
* Don't trigger OOB on partially-queued requests
* Don't accept new connections during OOB
* Move tests to `TestOutOfBandServer`
@wjordan wjordan force-pushed the wjordan:improve-oob-hook branch from bad2be5 to d416d0c Apr 26, 2020
end
app_wait = options.delete(:app_wait)
app = ->(_) do
raise 'OOB conflict' if in_oob.locked?

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Apr 27, 2020

Member

Nice, I like that approach.

@nateberkopec
Copy link
Member

nateberkopec commented Apr 27, 2020

Tests are 😍

@nateberkopec nateberkopec merged commit 774c460 into puma:master Apr 30, 2020
2 of 3 checks passed
2 of 3 checks passed
build build
Details
ubuntu 2.2
Details
ubuntu 2.3
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.