Fix out_of_band hook #2218

wjordan · 2020-04-07T23:14:38Z

Description

Fixes #2177. (Thanks @GuiTeK for tracking down this bug!)

Modifies the out_of_band hook implementation in the Server class to ensure that hooks are fired as expected when the server is configured with >1 max threads.

Compare to #2178, #2180. This implementation is pretty simple. It changes behavior slightly by executing the hooks on the same thread that finished processing the last request (instead of the main acceptor thread), so the server can continue accepting incoming requests if any arrive while the out-of-band hooks are still executing.

I've also added a couple of server tests to ensure out-of-band hooks fire as expected in two scenarios (sequential and parallel requests).

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.

MSP-Greg · 2020-04-07T23:38:59Z

I just switched to a different 'apt-get' server, so Ubuntu should consistently install ragel, I think.

nateberkopec · 2020-04-08T03:18:37Z

lib/puma/thread_pool.rb

@@ -79,6 +79,10 @@ def pool_capacity
      waiting + (@max - spawned)
    end

+    def busy_threads
+      @spawned - @waiting + @todo.size


I think we need to wait for the @mutex here if not already held. This will work on MRI but won't work on JRuby/etc.

Done in ff28636 - not 100% sure if this approach (calling @mutex.owned? in a with_mutex wrapper method) has any small perf implications we care about. Alternative would be to not use a method at all, or introduce a in_mutex argument.

nateberkopec · 2020-04-08T03:20:30Z

lib/puma/dsl.rb

@@ -494,8 +494,9 @@ def after_worker_fork(&block)

    # Code to run out-of-band when the worker is idle.
    # These hooks run immediately after a request has finished
-    # processing and there are no busy threads on the worker.
-    # The worker doesn't accept new requests until this code finishes.


Whoops, that wasn't ever true, was it?

Also, Q: should this be the actual behavior? Is it really OOB otherwise?

Whoops, that wasn't ever true, was it?

The oob hook previously ran from the acceptor thread, so I think it was true at least for the case where it was working correctly (max threads 1).

should this be the actual behavior? Is it really OOB otherwise?

Hmm, I went back and forth on whether it's better to keep accepting new requests while an OOB hook is running. (Might depend on the hook's use case?) For GC optimization at least, I thought continuing to accept requests could potentially be better under high load (though the MRI GVL could make it a wash). But on second thought, not accepting new requests is probably safer for light loads especially while request-balancing is still uneven. Plus it is 'really OOB' for other use-cases. I can change this to run on the acceptor thread as before.

Updated in 7b36391

nateberkopec · 2020-04-08T03:23:59Z

Only Q for me is: should the OOB hook work the way it was advertised? "No other requests will be processed until the hook finishes".

Both approaches have the potential to add latency to requests.

nateberkopec · 2020-04-09T06:21:02Z

Hm. I think that if the method is called oob, it needs to actually be oob - so:

Grab the threadpool mutex. If we are the only busy thread, then:
Stop accepting requests.
Run callbacks, return.
Start accepting requests.

nateberkopec · 2020-04-15T00:05:25Z

@wjordan thoughts on #2218 (comment)?

Add server tests to ensure oob hooks fire as expected.

Prevent overlapping requests from being processed during out of band hook.

nateberkopec · 2020-04-16T01:01:00Z

Oh OK, that's definitely better than interacting with the threadpool 👍

ayufan · 2020-04-23T17:21:54Z

lib/puma/server.rb

@@ -439,6 +436,12 @@ def process_client(client, buffer)
        rescue StandardError => e
          @events.unknown_error self, e, "Client"
        end
+
+        if @options[:out_of_band]
+          @thread_pool.with_mutex do


Does this actually create a mutex now on thread_pool? The OOB hook is executed within a mutex, rather than dispatched to be executed. Does it mean that it creates a contention now due to mutex, till OOB is executed, pretty much similar to before, stopping acceptance thread.

This also, connected with a test #test_out_of_band_stream creates a very strong dependency on execution: The test assumes that all connections are in fact processed concurrently, but this might not always be true, ~~because the test does fail if we assume that puma runs with threads=1~~. ~~The test does fail in this condition, as this is being executed on down failing edge of the event.~~

wjordan mentioned this pull request Apr 7, 2020

out_of_band hook not working when using multiple threads #2177

Closed

wjordan force-pushed the out_of_band_fix branch 2 times, most recently from 4f8feb8 to 5a49f22 Compare April 7, 2020 23:38

nateberkopec added bug waiting-for-review Waiting on review from anyone labels Apr 8, 2020

nateberkopec reviewed Apr 8, 2020

View reviewed changes

nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Apr 8, 2020

wjordan force-pushed the out_of_band_fix branch 2 times, most recently from c1092b3 to ff28636 Compare April 8, 2020 06:06

nateberkopec mentioned this pull request Apr 9, 2020

Fix out_of_band hook never executed when multiple worker threads (clo… #2180

Closed

8 tasks

Fix out_of_band hook

17ea126

Add server tests to ensure oob hooks fire as expected.

wjordan force-pushed the out_of_band_fix branch from 6be5e0d to 17ea126 Compare April 15, 2020 03:39

Use mutex for out of band hook

97e9d06

Prevent overlapping requests from being processed during out of band hook.

wjordan force-pushed the out_of_band_fix branch from 64db4f0 to 97e9d06 Compare April 15, 2020 21:21

nateberkopec merged commit df9cdc7 into puma:master Apr 16, 2020

ayufan reviewed Apr 23, 2020

View reviewed changes

ayufan mentioned this pull request Apr 23, 2020

Inject small delay for busy workers to improve requests distribution #2079

Merged

8 tasks

wjordan mentioned this pull request Apr 24, 2020

Improve OOB hook #2230

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix out_of_band hook #2218

Fix out_of_band hook #2218

wjordan commented Apr 7, 2020 •

edited

MSP-Greg commented Apr 7, 2020 •

edited

nateberkopec Apr 8, 2020 •

edited

wjordan Apr 8, 2020

nateberkopec Apr 8, 2020

nateberkopec Apr 8, 2020

wjordan Apr 8, 2020

wjordan Apr 8, 2020

nateberkopec commented Apr 8, 2020

nateberkopec commented Apr 9, 2020

nateberkopec commented Apr 15, 2020

nateberkopec commented Apr 16, 2020

ayufan Apr 23, 2020 •

edited

Fix out_of_band hook #2218

Fix out_of_band hook #2218

Conversation

wjordan commented Apr 7, 2020 • edited

Description

Your checklist for this pull request

MSP-Greg commented Apr 7, 2020 • edited

nateberkopec Apr 8, 2020 • edited

Choose a reason for hiding this comment

wjordan Apr 8, 2020

Choose a reason for hiding this comment

nateberkopec Apr 8, 2020

Choose a reason for hiding this comment

nateberkopec Apr 8, 2020

Choose a reason for hiding this comment

wjordan Apr 8, 2020

Choose a reason for hiding this comment

wjordan Apr 8, 2020

Choose a reason for hiding this comment

nateberkopec commented Apr 8, 2020

nateberkopec commented Apr 9, 2020

nateberkopec commented Apr 15, 2020

nateberkopec commented Apr 16, 2020

ayufan Apr 23, 2020 • edited

Choose a reason for hiding this comment

wjordan commented Apr 7, 2020 •

edited

MSP-Greg commented Apr 7, 2020 •

edited

nateberkopec Apr 8, 2020 •

edited

ayufan Apr 23, 2020 •

edited