forked from amnong/easywebdav
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Correctly manage connection reuse #3
Comments
Sounds reasonable. I'm ok with both suggestions. Feel free top open PR. |
By the way I see another approach already suggested to original lib: amnong@8f1408c |
That patch is incomplete (for instance, "download" will not reuse connection) and far more intrusive and non obvious that my proposal. I will create a pull request. |
Merged
PR merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Although technically "requests" library allows connection reuse, current use in "easywebdav2" precludes it.
The reason is simple: The requests session is tagged as "stream=True" and:
Most of the time, the body is not read at all, only the status code is checked. Since the body is not read and "stream=True", the connection can NOT be reused.
Even when the body is read, like in "download", we must call "req.close()" explicitly to give the connection back to the pool.
Suggestion:
Do NOT create the session with "stream=True". In this way, the request is completely read and the connection is back to the pool automatically.
In the few cases that stream reading is convenient, like in "download", do a "stream=True" request explicitly and manage it correctly.
If you agree with this plan, I can provide a pull request.
Thanks.
The text was updated successfully, but these errors were encountered: