csernazs / pytest-httpserver

Http server for pytest to test http clients
MIT License
213 stars 28 forks source link

Race Condition Prevents HTTP Server from Exiting #347

Closed TheTechromancer closed 1 week ago

TheTechromancer commented 1 month ago

Hi, first off thanks for making this library; it's very useful :)

For some time now, I've been running into an elusive race condition that prevents the pytest process from exiting. It only happens in rare cases after cancellation of async tasks, and it's been a real pain to track down.

At first I thought it was a race condition in httpx, since I've discovered similar bugs in the past. But in this case, the issue seems to be specific to pytest-httpserver.

Here is the code to reproduce:

import asyncio
import httpx
import traceback
from pytest_httpserver import HTTPServer

async def make_request(client, url):
    try:
        await client.get(url)
    except Exception:
        print(f"Request failed: {traceback.format_exc()}")

async def main():
    with HTTPServer() as httpserver:
        httpserver.expect_request("/").respond_with_data("")

        async with httpx.AsyncClient() as client:
            print('Creating tasks')
            tasks = []
            for i in range(100):
                tasks.append(asyncio.create_task(make_request(client, httpserver.url_for("/"))))

            print('Cancelling tasks')
            for task in tasks:
                await asyncio.sleep(0)
                task.cancel()

            print('Awaiting tasks')
            for i, task in enumerate(tasks):
                try:
                    await task
                except asyncio.CancelledError:
                    pass
            print('Finished awaiting tasks')
        print('Closed httpx client')
    print('Closed httpserver')

asyncio.run(main())

This should produce an httpx.ReadTimeout error, and the httpserver should fail to exit. I've tested in on Linux, Python 3.9-3.12.

Thanks!

csernazs commented 1 month ago

Hi @TheTechromancer ,

Thanks!

I could reproduce your issue. Based on strace output it seems httpx behaves correcty by closing the connections, but I could not find why the reason the server is not stopping. The server is single-threaded so the shutdown is done in a way that a flag is set to True, then some event (or other synchronization primitive) is defined. Later on, once the server finished processing a request, it checks for this flag and notifies the waiting thread via this synchronization primitive. This means if the server is processing a request, it cannot handle the shutdown event. As httpx is seemingly closing the sockets correctly, this might be a bug in the server.

As a workaround you can now start the threaded server (a new feature in 1.0.11), in such case each request is served in a new thread, so stopping the server is much quicker and it is free of this mechanism.

So you can try replacing the server call to this:

with HTTPServer(threaded=True) as httpserver:

This worked for me. Could you also try this? This also performed much better, I think it is better to use this to serve requests made not sequentially (eg what your async code does).

Zsolt

TheTechromancer commented 1 month ago

@csernazs thanks for the quick response! After some quick testing, that workaround seems to fix the issue. This bug has caused me so much pain, you have no idea how happy this makes me lol.

csernazs commented 1 month ago

Thanks! I'm happy that I could help :)

I think I'll do additional investigation on this later. pytest-httpserver uses werkzeug which then uses socketserver.py from stdlib, so as a maintainer for pytest-httpserver I rely on them to do the heavy lifting.

Also, interestingly:

  1. if you reduce the 100 to some lower value, the test pass
  2. If you add some sleep to here:
            for i in range(100):
                tasks.append(asyncio.create_task(make_request(client, httpserver.url_for("/"))))

             await asyncio.sleep(10) # <--- here

            print("Cancelling tasks")

then this also fixes it..

So there will be definitely a race condition..