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

Set enable_cleanup_closed=True to drop TLS connections without a shutdown #468

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

snemes
Copy link
Contributor

@snemes snemes commented Aug 3, 2023

Description

AsyncOpenSearch seems to leak TLS connections due to a missing parameter in aiohttp.TCPConnector.

Issues Resolved

This fixes #172 and was also fixed "upstream" in this issue elastic/elasticsearch-py#1910.
Also seems to be related to this aio-libs/aiohttp#6490 (which is probably the root cause of the issue).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…down

AsyncOpenSearch seems to leak TLS connections due to a missing parameter in `aiohttp.TCPConnector`. This causes opensearch-project#172 and was also fixed "upstream" in this issue elastic/elasticsearch-py#1910. 

Signed-off-by: Sandor Nemes <snemes@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #468 (2470d48) into main (dcb79cc) will decrease coverage by 0.07%.
Report is 1 commits behind head on main.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
- Coverage   71.69%   71.62%   -0.07%     
==========================================
  Files          87       87              
  Lines        7875     7881       +6     
==========================================
- Hits         5646     5645       -1     
- Misses       2229     2236       +7     
Files Coverage Δ
opensearchpy/__init__.py 100.00% <ø> (ø)
opensearchpy/_async/helpers/document.py 57.38% <100.00%> (ø)
opensearchpy/_async/helpers/index.py 60.80% <100.00%> (ø)
opensearchpy/_async/http_aiohttp.py 83.33% <100.00%> (ø)
opensearchpy/connection/async_connections.py 95.91% <100.00%> (+0.17%) ⬆️
opensearchpy/connection_pool.py 89.74% <100.00%> (ø)
opensearchpy/helpers/actions.py 44.22% <ø> (ø)
opensearchpy/helpers/asyncsigner.py 95.65% <100.00%> (ø)
opensearchpy/helpers/field.py 93.37% <100.00%> (ø)
opensearchpy/helpers/index.py 64.50% <100.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

@dblock
Copy link
Member

dblock commented Aug 3, 2023

Thanks! Could you please add a line to CHANGELOG and write a unit test (ideally showing that the leak is fixed, but at least that the parameter is passed through).

@snemes
Copy link
Contributor Author

snemes commented Aug 3, 2023 via email

Signed-off-by: Sandor Nemes <snemes@users.noreply.github.com>
@dblock
Copy link
Member

dblock commented Aug 3, 2023

Sure, I can add a line to the changelog, but I won't have time to write a unit test for this, sorry about that.

On Thu, Aug 3, 2023, 17:24 Daniel (dB.) Doubrovkine < @.> wrote: Thanks! Could you please add a line to CHANGELOG and write a unit test (ideally showing that the leak is fixed, but at least that the parameter is passed through). — Reply to this email directly, view it on GitHub <#468 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABP4VHX5QX7ETYREDSWGW7LXTO7DLANCNFSM6AAAAAA3C4WYHU . You are receiving this because you authored the thread.Message ID: @.>

We'll have to wait for one to merge this. Thanks for your help, hopefully someone can finish the PR.

@saimedhi
Copy link
Collaborator

@snemes, Could you please try to write a test when you have a moment? Thank you!

Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
@dblock
Copy link
Member

dblock commented Nov 9, 2023

I tried to concoct a test for this but it became rather convoluted, so I gave up. Merging this as is. The documentation in https://docs.aiohttp.org/en/stable/client_reference.html is convincing enough that this is a harmless improvement that times out unclosed SSL connections and ES client already merged the same including in elastic/elastic-transport-python@57a2c18.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @snemes for contributing!

@dblock dblock merged commit 56606ed into opensearch-project:main Nov 9, 2023
53 of 54 checks passed
roma2023 pushed a commit to roma2023/opensearch-py that referenced this pull request Dec 28, 2023
…down (opensearch-project#468)

* Set enable_cleanup_closed=True to drop TLS connections without a shutdown

AsyncOpenSearch seems to leak TLS connections due to a missing parameter in `aiohttp.TCPConnector`. This causes opensearch-project#172 and was also fixed "upstream" in this issue elastic/elasticsearch-py#1910.

Signed-off-by: Sandor Nemes <snemes@users.noreply.github.com>

* Update CHANGELOG.md

Signed-off-by: Sandor Nemes <snemes@users.noreply.github.com>

---------

Signed-off-by: Sandor Nemes <snemes@users.noreply.github.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
Signed-off-by: roma2023 <romasaparhan19@gmail.com>
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

Successfully merging this pull request may close these issues.

[BUG] Connection Leak
3 participants