Close a connection if an unexpected exception occurs while sending a request #2828

brycedrennan · 2018-03-13T04:26:55Z

What do these changes do?

Close a connection if an unexpected exception occurs while sending a request

Are there changes in behavior for the user?

No.

Related issue number

Resolves #2827

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

codecov-io · 2018-03-13T05:07:27Z

Codecov Report

Merging #2828 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2828   +/-   ##
=======================================
  Coverage   97.96%   97.96%           
=======================================
  Files          39       39           
  Lines        7426     7426           
  Branches     1305     1305           
=======================================
  Hits         7275     7275           
  Misses         48       48           
  Partials      103      103

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9108698...e902045. Read the comment docs.

pfreixes · 2018-03-13T09:13:38Z

@asvetlov and @brycedrennan some comments on this MR and the issue related

Using the context manager way of sending a request rather than use an await this issue shouldn't appear, am I right ?
And specifically of that MR, Why close and not release? the context manager uses as a teardown the release method.

asvetlov · 2018-03-13T10:20:49Z

@pfreixes

Yes, from my understanding context manager should close connection on exception
release returns a connection back to pool. If exception has occurred the connection is in undefined state: it can have not-read-yet data etc. Dropping the connection entirely by close() is safer.

brycedrennan · 2018-03-13T10:29:15Z

@pfreixes

It doesn't appear to help to use the context manager. I modified the test to use the context manager and it still has trouble without the fix.
I don't know the difference. I used close because thats what was used to handle exceptions during .start

kxepal

LGFM, but there is one little bit...

kxepal · 2018-03-13T10:31:41Z

tests/test_client_session.py

@@ -379,6 +380,42 @@ def test_context_manager(connector, loop):
    assert e.strerror == err.strerror


+async def test_close_conn_on_error(create_session):
+    class UnexpectedException(Exception):


Your try/except block catches BaseExceptions, but here you test just for Exception. It's easy to change the code and break the behavior, but not the tests.

kxepal · 2018-03-13T10:37:07Z

aiohttp/client.py

+                            resp = await req.send(conn)
+                        except BaseException:
+                            conn.close()
+                            raise


It seems we're don't the same thing twice. How about to regroup try/except a bit?

try: resp = await req.send(conn) try: await resp.start(conn, read_unit_eof) except: resp.close() raise except: conn.close() raise

kxepal · 2018-03-13T10:42:25Z

And sign question about context managers. It's strange that they doesn't helps.

kxepal · 2018-03-13T10:51:44Z

@brycedrennan Thanks!

Change test to use context manager, BaseException

pfreixes · 2018-03-13T10:54:49Z

Checked, sorry for the noise but finally I was able to connect all of the dots, I would like to propose a silly change in this MR.

The context manager is not able to close the connection because there is no response yet and won't be able to make it. My bet is that with your example @brycedrennan related with the context manager you are having another exception raised within the original exception.

Basically, the change that I propose is adding a check to make sure that the resp attribute is present [1].

Thoughts @asvetlov and @brycedrennan ?

[1] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L790

brycedrennan · 2018-03-13T11:01:14Z

@pfreixes __aexit__ is never called because the exception is thrown during __aenter__. I just put some breakpoints in the debugger, and confirmed by adding a try/except into the __aenter__.

Also the test is catching the specific exception that was thrown.

pfreixes · 2018-03-13T11:32:19Z

@brycedrennan my fault I was totally confused, the exception happens in the __aenter__ so as you said the __aexit__ is not called.

Forget my comment. The response and the connection teardown in case of exception within Client.__request can't rely on the context manager.

asvetlov · 2018-03-13T19:22:06Z

Everything looks good, will merge tomorrow if nobody objects.
The PR should be backported to 3.0 branch and new 3.0.x release should be published (I'll do it myself).
@brycedrennan thanks for your work!

…request (#2828)

lock · 2019-10-28T08:03:42Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

Close a connection when an unexpected exception occurs during request.

614dbb2

Merge branch 'master' into header-error-cleanup

254a1f2

asvetlov approved these changes Mar 13, 2018

View reviewed changes

kxepal requested changes Mar 13, 2018

View reviewed changes

kxepal approved these changes Mar 13, 2018

View reviewed changes

Regroup exception catching logic

e902045

Change test to use context manager, BaseException

asvetlov merged commit 4534021 into aio-libs:master Mar 14, 2018

asvetlov pushed a commit that referenced this pull request Mar 14, 2018

Close a connection if an unexpected exception occurs while sending a …

1b45e57

…request (#2828)

brycedrennan deleted the header-error-cleanup branch March 15, 2018 05:22

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close a connection if an unexpected exception occurs while sending a request #2828

Close a connection if an unexpected exception occurs while sending a request #2828

brycedrennan commented Mar 13, 2018

codecov-io commented Mar 13, 2018 •

edited

pfreixes commented Mar 13, 2018

asvetlov commented Mar 13, 2018

brycedrennan commented Mar 13, 2018 •

edited

kxepal left a comment

kxepal Mar 13, 2018

brycedrennan Mar 13, 2018

kxepal Mar 13, 2018

brycedrennan Mar 13, 2018

kxepal commented Mar 13, 2018

kxepal commented Mar 13, 2018

pfreixes commented Mar 13, 2018 •

edited

brycedrennan commented Mar 13, 2018 •

edited

pfreixes commented Mar 13, 2018

asvetlov commented Mar 13, 2018

lock bot commented Oct 28, 2019

Close a connection if an unexpected exception occurs while sending a request #2828

Close a connection if an unexpected exception occurs while sending a request #2828

Conversation

brycedrennan commented Mar 13, 2018

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

codecov-io commented Mar 13, 2018 • edited

Codecov Report

pfreixes commented Mar 13, 2018

asvetlov commented Mar 13, 2018

brycedrennan commented Mar 13, 2018 • edited

kxepal left a comment

Choose a reason for hiding this comment

kxepal Mar 13, 2018

Choose a reason for hiding this comment

brycedrennan Mar 13, 2018

Choose a reason for hiding this comment

kxepal Mar 13, 2018

Choose a reason for hiding this comment

brycedrennan Mar 13, 2018

Choose a reason for hiding this comment

kxepal commented Mar 13, 2018

kxepal commented Mar 13, 2018

pfreixes commented Mar 13, 2018 • edited

brycedrennan commented Mar 13, 2018 • edited

pfreixes commented Mar 13, 2018

asvetlov commented Mar 13, 2018

lock bot commented Oct 28, 2019

codecov-io commented Mar 13, 2018 •

edited

brycedrennan commented Mar 13, 2018 •

edited

pfreixes commented Mar 13, 2018 •

edited

brycedrennan commented Mar 13, 2018 •

edited