csernazs / pytest-httpserver

Http server for pytest to test http clients
MIT License
209 stars 27 forks source link

Enable processing concurrent requests in separate threads #324

Closed delthas closed 2 months ago

delthas commented 2 months ago

Closes: https://github.com/csernazs/pytest-httpserver/issues/323

No risk of leaving threads behind: werkzeug.ThreadedWSGIServer uses socketserver.ThreadingMixIn, which adds a self._threads.join() in its server_close. self._threads.join() is called before self.server.serve_forever() returns, so before self.server_thread.join() returns, so before self.stop() returns.

The wording "handle concurrent requests in separate threads" was taken from the ThreadedWSGIServer documentation.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.64%. Comparing base (8e66133) to head (3cb7af9).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #324 +/- ## ======================================= Coverage 95.63% 95.64% ======================================= Files 4 4 Lines 619 620 +1 ======================================= + Hits 592 593 +1 Misses 27 27 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

csernazs commented 2 months ago

Thank you for checking! 🙏

Code also looks good. 💯

Could you try adding a test for it? IMO there would be a separate httpserver fixture created for that. I just like to see that happy-path works in a simplest way, eg a handler which returns the thread name and we compare the thread names are not equal or something like that.

If you don't want to do that, I can do it as well.

Zsolt

csernazs commented 2 months ago

hi @delthas

I pushed tests for your PR, I hope you don't mind it - I force-pushed your branch as I had to update ruff in pre-commit, so I rebased your PR to origin/master.

I'll write a howto and update docs in a separate PR, but I'll let you some time (a few days) to review the tests. Feel free to share if you have any better ideas for the tests. Probably it could be made better by having proper synchronization primitives, at the moment it could be flaky if the client sends requests slower.

Thanks, Zsolt

delthas commented 2 months ago

Hi,

Thanks for working on this MR and adding the tests.

I just reviewed the tests and they look good to me; this is exactly what I'd do (and what I did on my end when I tested the feature): add a handler that sleeps, run many connections, check that the time is less than handlind them sequentially. On my end I used multiprocessing ThreadPool submit to start all the requests in parallel; here I see we handle them sequentially but that should also work.

csernazs commented 2 months ago

Thanks!

It just came to my mind that in pytest-httpserver we use a session scoped fixture so having that "werzeug joins the threads on stop" possibly won't be sufficient - as the server is not stopped and started between the tests (in my test it is not tested).

I just want to verify this before the release. I think we should add some join call to the clear method.

Theoretically there would be no issue leaving a thread behind as the handlers registry gets cleared between tests, so there's no chance the old handler getting called, but I feel it would be better to not leave behind any running thread.

Zsolt

csernazs commented 2 months ago

hi @delthas ,

I checked the issue described in the last comment. This leaves behind the running handler threads, that's apparent, but I found no way to wait on those threads. Given the current state of socketserver.py it is not possible to query the running threads safely.

But this is not a big issue as those handler threads will exit at least at the point when the server is stopped, and they won't receive any new requests as the handlers "registry" is cleared between the tests. I will make a note about this in the documentation, though.

So this PR can be merged.

Thanks for the contribution,

Zsolt

delthas commented 2 months ago

Thanks!