Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.85%. Comparing base (996ad00) to head (5a405b8).
Report is 7 commits behind head on master.

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##           master   #11148      +/-   ##
==========================================
- Coverage   98.86%   98.85%   -0.01%     
==========================================
  Files         131      131              
  Lines       42737    42966     +229     
  Branches     2308     2314       +6     
==========================================
+ Hits        42250    42476     +226     
- Misses        337      340       +3     
  Partials      150      150              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CodSpeed Performance Report

Merging #11148 will not alter performance

_{Comparing abort_on_ssl_timeout_0 (5a405b8) with master (996ad00)}

Summary

✅ 59 untouched benchmarks

I'm still unclear on your changes from #11114 though. The changelog says it's expected behaviour on TLS 1.3. If that's true, we shouldn't be waiting at all, if we expect it to never work. I thought it was only misbehaving servers which don't close the connection properly.

I'm still unclear on your changes from #11114 though. The changelog says it's expected behaviour on TLS 1.3. If that's true, we shouldn't be waiting at all, if we expect it to never work. I thought it was only misbehaving servers which don't close the connection properly.

TLS 1.2 is required to acknowledge immediately. With TLS 1.3 it isn't required but should acknowledge quickly, but it is allowed to flush the buffer first.

In practice though misbehaving servers don't flush the buffer and never acknowledge, leaving the user in a state they can do nothing about.

I'm almost tempted to change the default to 0 and simply abort the connections since there isn't much value in waiting at all if we are closing the connector. I just worry that we don't flush the buffer in that case, but since the connector is closing, does it matter ?

The more I think about it, the more I'm leaning to making 0 the default as I can't see any reason we would need to worry about flushing the buffer if we are done with the session/connector

The more I think about it, the more I'm leaning to making 0 the default as I can't see any reason we would need to worry about flushing the buffer if we are done with the session/connector

I'm not too familiar with what the risks are. Feels like some misbehaving servers should be fixed...

The key risk is we we might not flush the read/write buffer, but if are are done with the session/connector anyways it should be fine to abort. I'm worried about the case where someone expects they have an event loop iteration remaining to read the buffered client data in another task before the connector closes.

It comes down to, is it reasonable to expect that someone would want to read the buffer after calling .close() on the connector.. I think the answer is no

So if we change to abort with 0, that means during normal operation we get the default python timeout in the background, and we abort() when the connector is closed. That actually sounds like the best of both worlds.

I guess my question is who's going to change it from the default? What problem would it fix, and how would a user know they want to change that default?

So if we change to abort with 0, that means during normal operation we get the default python timeout in the background, and we abort() when the connector is closed. That actually sounds like the best of both worlds.

I guess my question is who's going to change it from the default? What problem would it fix, and how would a user know they want to change that default?

I can't actually envision a use case where they would want anything other than the new default in this PR (0). Waiting 0.1s to close the connector is very undesirable when you are frequently creating and destroying them which seems to be a much more common use case than we have expected.

So, it kind of feels to me like the parameter should be removed again..

So, it kind of feels to me like the parameter should be removed again..

Yeah I think so. I'll deprecate it for removal in 4.0

Backport to 3.12: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 86a9a38 on top of patchback/backports/3.12/86a9a3827c876acfd32886bd51b49df7f5f1925e/pr-11148

Backporting merged PR #11148 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.12/86a9a3827c876acfd32886bd51b49df7f5f1925e/pr-11148 upstream/3.12

4. Now, cherry-pick PR Abort ssl connections on close when ssl_shutdown_timeout is 0 #11148 contents into that branch:

   $ git cherry-pick -x 86a9a3827c876acfd32886bd51b49df7f5f1925e

   If it'll yell at you with something like fatal: Commit 86a9a3827c876acfd32886bd51b49df7f5f1925e is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 86a9a3827c876acfd32886bd51b49df7f5f1925e

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Abort ssl connections on close when ssl_shutdown_timeout is 0 #11148 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.12/86a9a3827c876acfd32886bd51b49df7f5f1925e/pr-11148

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Backport to 3.13: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 86a9a38 on top of patchback/backports/3.13/86a9a3827c876acfd32886bd51b49df7f5f1925e/pr-11148

Backporting merged PR #11148 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.13/86a9a3827c876acfd32886bd51b49df7f5f1925e/pr-11148 upstream/3.13

4. Now, cherry-pick PR Abort ssl connections on close when ssl_shutdown_timeout is 0 #11148 contents into that branch:

   $ git cherry-pick -x 86a9a3827c876acfd32886bd51b49df7f5f1925e

   If it'll yell at you with something like fatal: Commit 86a9a3827c876acfd32886bd51b49df7f5f1925e is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 86a9a3827c876acfd32886bd51b49df7f5f1925e

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Abort ssl connections on close when ssl_shutdown_timeout is 0 #11148 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.13/86a9a3827c876acfd32886bd51b49df7f5f1925e/pr-11148

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.