search

profusion / sgqlc

Simple GraphQL Client
https://sgqlc.readthedocs.io/
ISC License
506 stars 85 forks source link

Connections in requests session are closed after each request #225

Closed reconman closed 1 year ago

reconman commented 1 year ago

🐜 Bug Report

Expected Behavior

When you use RequestsEndpoint(..., session=requests.Session()), sgqlc should reuse the connections for better performance.

Current Behavior

RequestsEndpoint closes the session after each request in this line: https://github.com/profusion/sgqlc/blob/f9107aadddc7052323cb99b6c00a0daaa8dedb2d/sgqlc/endpoint/requests.py#L181

Here's the call stack from Visual Studio Code when I debugged why connections were closed: image

The with statement automatically closes the session every time.

Possible Solution

Don't close the session if it was provided by the user.

For the other case of RequestsEndpoint creating one, you can't guarantee that the user will reuse the same endpoint object, so they may accidentally create 1000s of open sessions. That's why I would keep the with for that case.

Steps to Reproduce

Change the Github Agile Dashboard example the following way:

  1. Replace the HTTPEndpoint with a RequestsEndpoint
  2. Add a requests.Session() to the constructor
  3. Enable urllib3.connectionpool debug logs
  4. Run the example

The log will be filled with urllib3.connectionpool Starting new HTTPS connection (1): api.github.com:443

Context (Environment)

I didn't notice the issue until I looked into the debug logs.

My script runs 1000 or more graphql queries in a row.

When I edited requests.py to not close the connection every time, the urllib3.connectionpool logs looked correct, but my script only got 1 second faster, so it doesn't seem to have a giant impact. But also there's much happening before the graphql queries so I can't reliable test the difference.

barbieri commented 1 year ago

would you mind providing a patch? I can review it, just make sure you run the git hooks (flake8, tests).

As for the implementation. I think it's feasible to create the session in the constructor if it's not provided (none):

   self.session = session or requests.Session()

Move the self.session.close() to be explicit in the destructor (__del__(), needs to be created).

Then we can opt to flag if it's owned (created locally) or passed... like only close() if it was created locally.

reconman commented 1 year ago

I can review it, just make sure you run the git hooks (flake8, tests).

If you approve my PR workflow run, those tests will automatically run.

I modified the source code in my poetry venv and everything worked as expected.