aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

`_release_connection` is always called 4x on a successful request

Open bdraco opened this issue 1 year ago • 6 comments

          I only found this because I was looking at function call counts..

I was trying to find why _release_connection is always called 4x per request

Screenshot 2024-12-01 at 6 57 37 PM

Originally posted by @bdraco in https://github.com/aio-libs/aiohttp/issues/10088#issuecomment-2510354243

bdraco avatar Dec 02 '24 00:12 bdraco

I'm curious how did you get such pretty graph? What tool do you use?

asvetlov avatar Dec 02 '24 11:12 asvetlov

The idea is: connection is released either on reaching EOF for response body or explicit resp.release() call. It complicates things a lot and I never liked this code, but it is the current state.

asvetlov avatar Dec 02 '24 11:12 asvetlov

I think if the writer is done (presumably the situation in most cases), then all but one of those calls are basically no-op, so shouldn't have much impact on performance (I guess this can be seen by the start path being the only one which exceeds 1%).

The __aexit__ path is the explicit release, this one is needed to ensure the connection is closed if the body was not fully read. The start path looks like it is run after the message has been fully read, which is probably the normal path. I don't think data_received is actually resulting in any calls to _release_connection(). I can't see any way that'd happen. The _wait_released path is ensuring that the connection is released before returning from resp.read(). This is probably the normal path when the body size exceeds the initial payload read (and not using the streaming API). Without this one, I think there can be a race condition (I believe we have tests covering it if you want to experiment).

I think that's 3 calls, rather than 4, unless I'm missing something.

Dreamsorcerer avatar Dec 02 '24 13:12 Dreamsorcerer

I think that's 3 calls, rather than 4, unless I'm missing something.

Its a bit confusing, but notice release on the left hand side calls it 40000 times so I think it is 4x but I haven't verified it yet. I only opened this issue because I ran out of weekend to work on it and didn't want to forget.

I've been looking at it because I can't figure out why on codspeed we see the cost of releasing the connection as more expensive than acquiring the connection. Usually something like that is the other way around.

bdraco avatar Dec 02 '24 14:12 bdraco

I'm curious how did you get such pretty graph? What tool do you use?

I generated it using cProfile, than I converted the cprof to callgrind.out.XXXXX using pyprof2calltree, and opened it in qcachegrind

bdraco avatar Dec 02 '24 14:12 bdraco

Its a bit confusing, but notice release on the left hand side calls it 40000 times so I think it is 4x but I haven't verified it yet. I only opened this issue because I ran out of weekend to work on it and didn't want to forget.

Ah, yes, the wait_for_close() method calls .release() a second time. In theory, you can probably remove that second call to .release(), but there might have been another race condition there. If the tests pass without that call, then it's probably fine.

Dreamsorcerer avatar Dec 02 '24 14:12 Dreamsorcerer