bpo-29406: asyncio SSL contexts leak sockets after calling close with certain servers by fafhrd91 · Pull Request #409 · python/cpython

fafhrd91 · 2017-03-03T00:58:36Z

Some servers does not complete ssl shutdown procedure, which cause socket leak on asyncio side.

the-knights-who-say-ni · 2017-03-03T00:58:37Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

fafhrd91 · 2017-03-03T00:59:42Z

@1st1 PR for http://bugs.python.org/issue29406
I need to add tests though

fafhrd91 · 2017-03-03T01:00:34Z

I actually signed CLA before I started to contribute to tulip/asyncio

1st1 · 2017-03-03T01:01:48Z

I actually signed CLA before I started to contribute to tulip/asyncio

I think you need to go to the bugs.python.org and add your GH username to your profile.

fafhrd91 · 2017-03-03T01:08:25Z

I dont remembre account that I used when signed cla. I just re-signed it

1st1 · 2017-03-03T05:35:41Z

Maybe you used OAuth?

fafhrd91 · 2017-03-03T23:17:22Z

my bugs.python.org details page shows "Contributor Form Received: Yes"

1st1 · 2017-03-03T23:23:03Z

Did you list your GH account name in your account on bpo?

fafhrd91 · 2017-03-03T23:23:28Z

yes

fafhrd91 · 2017-03-03T23:24:57Z

http://bugs.python.org/user17178

1st1 · 2017-03-03T23:26:56Z

@brettcannon something weird is going on here with @the-knights-who-say-ni bot.

@fafhrd91 Can you rebase your PR against master branch? And then add "needs backport to 3.6" and 3.5 tags.

fafhrd91 · 2017-03-03T23:37:27Z

rebased, but I can not add labels

1st1 · 2017-03-03T23:40:33Z

I would rename this method to _on_shutdown_timeout.

1st1 · 2017-03-03T23:43:47Z

To make the code more clear I would set self._transport = None in _finalize and _fatal_error methods.

fafhrd91 · 2017-03-04T01:02:34Z

added simple unit tests

brettcannon · 2017-03-04T01:27:53Z

@1st1 there was nothing weird going on; you didn't remove the "CLA not signed" to retrigger the bot to check for the new CLA signing. In the end the synchronizing of the PR triggered the bot again and picked up on the signing.

1st1 · 2017-03-06T21:38:01Z

little nit: I'd add a newline before this if.

1st1 · 2017-03-06T21:39:09Z

Why is it 5 seconds? Is this some timeout value defined in some SSL RFC?

1st1 · 2017-03-12T19:35:24Z

@fafhrd91 ping

fafhrd91 · 2017-03-12T19:37:03Z

5 seconds just some number

misg · 2017-04-03T17:12:09Z

You may want to see #981

fafhrd91 · 2017-04-03T17:15:05Z

@misg I think this is slightly different issue, #480 is related to #981

misg · 2017-04-03T18:55:53Z

@fafhrd91 I honestly don't know. In my case (with the server/client described in my PR), aiohttp.connector._cleanup_closed is always called and it is that function that calls asyncio.sslproto.SSLTransport._abort so I think I'm in the context where the ssl server doesn't complete ssh shutdown procedure (I actually didn't test with a ssl server that completes ssh shutdown procedure)

fafhrd91 · 2017-06-09T23:02:32Z

@1st1 done

fafhrd91 · 2017-06-09T23:03:23Z

wait

fafhrd91 · 2017-06-09T23:28:34Z

@1st1 restored

1st1 · 2017-06-09T23:29:12Z

Can you add a NEWS entry?

fafhrd91 · 2017-06-09T23:37:24Z

added

1st1 · 2017-06-10T02:50:31Z

@1st1 restored

unittests don't seem to pass :(

1st1 · 2017-06-10T03:30:12Z

Should we also cleanup _shutdown_timeout_handle here?

Should fatal_error just call finalize?

I don't think that makes sense.

added cleanup code for _shutdown_timeout_handle, it is possible to get here while handle still active

…rvers

… certain servers (#409) (cherry picked from commit a608d2d)

…e with certain servers (GH-409) * asyncio SSL contexts leak sockets after calling close with certain servers * cleanup _shutdown_timeout_handle on _fatal_error. (cherry picked from commit a608d2d)

… certain servers (#409) (cherry picked from commit a608d2d)

…e with certain servers (GH-409) * asyncio SSL contexts leak sockets after calling close with certain servers * cleanup _shutdown_timeout_handle on _fatal_error. (cherry picked from commit a608d2d)

…e with certain servers (GH-409) (#2062) * bpo-29406: asyncio SSL contexts leak sockets after calling close with certain servers (#409) (cherry picked from commit a608d2d) * [3.6] bpo-29406: asyncio SSL contexts leak sockets after calling close with certain servers (GH-409) * asyncio SSL contexts leak sockets after calling close with certain servers * cleanup _shutdown_timeout_handle on _fatal_error. (cherry picked from commit a608d2d)

…e with certain servers (GH-409) (#2063) * bpo-29406: asyncio SSL contexts leak sockets after calling close with certain servers (#409) (cherry picked from commit a608d2d) * [3.5] bpo-29406: asyncio SSL contexts leak sockets after calling close with certain servers (GH-409) * asyncio SSL contexts leak sockets after calling close with certain servers * cleanup _shutdown_timeout_handle on _fatal_error. (cherry picked from commit a608d2d)

…ose with certain servers (#409)" This reverts commit a608d2d.

…ing close with certain servers (GH-409) (#2062)" This reverts commit 6e14fd2.

…ing close with certain servers (GH-409) (#2063)" This reverts commit 1395c58.

…ing close with certain servers (GH-409) (#2062)" (#2112) This reverts commit 6e14fd2.

…ing close with certain servers (GH-409) (#2063)" (#2113) This reverts commit 1395c58.

…ose with certain servers (#409)" (#2111) This reverts commit a608d2d.

Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.6.2 to 3.7.2. - [Release notes](https://github.com/aio-libs/aiohttp/releases) - [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst) - [Commits](aio-libs/aiohttp@v3.6.2...v3.7.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

Per supervisor 19:13:18Z artefact request + [arch_completeness_disclaimer] durable-falsifiable rule. Expands the existing c22b-mech audit comment in MakeDecref else-branch (around the c22b-mech _Py_Dealloc bridge call) into a full 16-site enumeration with per-site disposition. Inlines the 6 active-rewired sites (c22b-mech site 611 + c23 site 599 + c24 kRaiseAwaitableError + c25 kInitFrameCellVars + c26 kSetCurrentAwaiter SP-only + c27 kStoreArrayItem SP-only — the latter two with disposition C provisional tag) and the 10 dead/conditional sites with their gating #ifdef / version macros explained: - 3 sites #ifdef Py_GIL_DISABLED (DEAD in default GIL build) - 1 site #ifdef Py_TRACE_REFS (DEAD in default build) - 5 sites #if PY_VERSION_HEX < 0x030C0000 (DEAD in 3.12 target) - 1 site #elif defined(ENABLE_LIGHTWEIGHT_FRAMES) (build-option-gated) Originally enumerated chat-only at theologian 13:29:55Z + supervisor disposition; converted to in-tree artefact for cross-session durability (per pythia python#409 audit-class observation 19:12:39Z + supervisor 19:13:18Z). Re-port becomes mandatory if Phoenix adopts a build config matching any of the gates (free-threaded GIL, TRACE_REFS, pre-3.12 compat, or ENABLE_LIGHTWEIGHT_FRAMES). No prod codegen change — comment-only. Authorized by supervisor 19:13:18Z artefact-class request.

the-knights-who-say-ni added the CLA not signed label Mar 3, 2017

fafhrd91 changed the title ~~asyncio SSL contexts leak sockets after calling close with certain servers~~ bpo-29406: asyncio SSL contexts leak sockets after calling close with certain servers Mar 3, 2017

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Mar 3, 2017

fafhrd91 changed the base branch from 3.6 to master March 3, 2017 23:36

1st1 added needs backport to 3.5 labels Mar 3, 2017

1st1 reviewed Mar 3, 2017

View reviewed changes

1st1 reviewed Mar 6, 2017

View reviewed changes

Comment thread Lib/asyncio/sslproto.py Outdated

1st1 Mar 6, 2017

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

little nit: I'd add a newline before this if.

1st1 reviewed Mar 6, 2017

View reviewed changes

Comment thread Lib/asyncio/sslproto.py Outdated

1st1 Mar 6, 2017

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

Why is it 5 seconds? Is this some timeout value defined in some SSL RFC?

1st1 self-assigned this Jun 10, 2017

1st1 reviewed Jun 10, 2017

View reviewed changes

fafhrd91 added 2 commits June 9, 2017 20:43

asyncio SSL contexts leak sockets after calling close with certain se…

b4eb6ae

…rvers

cleanup _shutdown_timeout_handle on _fatal_error

31d625a

1st1 merged commit a608d2d into python:master Jun 10, 2017

1st1 pushed a commit that referenced this pull request Jun 10, 2017

bpo-29406: asyncio SSL contexts leak sockets after calling close with…

009abbe

… certain servers (#409) (cherry picked from commit a608d2d)

1st1 pushed a commit that referenced this pull request Jun 10, 2017

bpo-29406: asyncio SSL contexts leak sockets after calling close with…

304b885

… certain servers (#409) (cherry picked from commit a608d2d)

1st1 pushed a commit that referenced this pull request Jun 11, 2017

Revert "bpo-29406: asyncio SSL contexts leak sockets after calling cl…

eb9bd66

…ose with certain servers (#409)" This reverts commit a608d2d.

1st1 mentioned this pull request Jun 11, 2017

Revert "bpo-29406: asyncio SSL contexts leak sockets after calling cl… #2111

Merged

1st1 pushed a commit that referenced this pull request Jun 11, 2017

Revert "[3.6] bpo-29406: asyncio SSL contexts leak sockets after call…

8ffe094

…ing close with certain servers (GH-409) (#2062)" This reverts commit 6e14fd2.

1st1 mentioned this pull request Jun 11, 2017

Revert "[3.6] bpo-29406: asyncio SSL contexts leak sockets after call… #2112

Merged

1st1 pushed a commit that referenced this pull request Jun 11, 2017

Revert "[3.5] bpo-29406: asyncio SSL contexts leak sockets after call…

71ada0b

…ing close with certain servers (GH-409) (#2063)" This reverts commit 1395c58.

1st1 mentioned this pull request Jun 11, 2017

Revert "[3.5] bpo-29406: asyncio SSL contexts leak sockets after call… #2113

Merged

1st1 pushed a commit that referenced this pull request Jun 11, 2017

Revert "[3.6] bpo-29406: asyncio SSL contexts leak sockets after call…

83d30bd

…ing close with certain servers (GH-409) (#2062)" (#2112) This reverts commit 6e14fd2.

1st1 pushed a commit that referenced this pull request Jun 11, 2017

Revert "[3.5] bpo-29406: asyncio SSL contexts leak sockets after call…

4e9dfe2

…ing close with certain servers (GH-409) (#2063)" (#2113) This reverts commit 1395c58.

1st1 pushed a commit that referenced this pull request Jun 11, 2017

Revert "bpo-29406: asyncio SSL contexts leak sockets after calling cl…

09663de

…ose with certain servers (#409)" (#2111) This reverts commit a608d2d.

Uh oh!

Uh oh!

Conversation

fafhrd91 commented Mar 3, 2017

Uh oh!

the-knights-who-say-ni commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

1st1 commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

1st1 commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

1st1 commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

1st1 commented Mar 3, 2017

Uh oh!

fafhrd91 commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fafhrd91 commented Mar 4, 2017

Uh oh!

brettcannon commented Mar 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Mar 12, 2017

Uh oh!

fafhrd91 commented Mar 12, 2017

Uh oh!

misg commented Apr 3, 2017

Uh oh!

fafhrd91 commented Apr 3, 2017

Uh oh!

misg commented Apr 3, 2017

Uh oh!

fafhrd91 commented Jun 9, 2017

Uh oh!

fafhrd91 commented Jun 9, 2017

Uh oh!

fafhrd91 commented Jun 9, 2017

Uh oh!

1st1 commented Jun 9, 2017

Uh oh!

fafhrd91 commented Jun 9, 2017

Uh oh!

1st1 commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone