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

_release_connection is always called 4x on a successful request #10089

Open
bdraco opened this issue Dec 2, 2024 · 6 comments
Open

_release_connection is always called 4x on a successful request #10089

bdraco opened this issue Dec 2, 2024 · 6 comments
Assignees

Comments

@bdraco
Copy link
Member

bdraco commented Dec 2, 2024

          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 #10088 (comment)

@bdraco bdraco self-assigned this Dec 2, 2024
@asvetlov
Copy link
Member

asvetlov commented Dec 2, 2024

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

@asvetlov
Copy link
Member

asvetlov commented Dec 2, 2024

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.

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Dec 2, 2024

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.

@bdraco
Copy link
Member Author

bdraco commented Dec 2, 2024

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
Copy link
Member Author

bdraco commented Dec 2, 2024

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

@Dreamsorcerer
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants