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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Mar 14, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
614dbb2
Close a connection when an unexpected exception occurs during request.
brycedrennan Mar 13, 2018
254a1f2
Merge branch 'master' into header-error-cleanup
asvetlov Mar 13, 2018
e902045
Regroup exception catching logic
brycedrennan Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
.cache
.coverage
.coverage.*
.direnv
.envrc
.idea
.installed.cfg
.noseids
Expand Down Expand Up @@ -48,4 +50,4 @@ virtualenv.py
.flake
.python-version
.pytest_cache
.vscode
.vscode
1 change: 1 addition & 0 deletions CHANGES/2827.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Close a connection if an unexpected exception occurs while sending a request
1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Boyi Chen
Brett Cannon
Brian C. Lane
Brian Muller
Bryce Drennan
Carl George
Cecile Tonglet
Chien-Wei Huang
Expand Down
6 changes: 5 additions & 1 deletion aiohttp/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,11 @@ async def _request(self, method, url, *,
tcp_nodelay(conn.transport, True)
tcp_cork(conn.transport, False)
try:
resp = await req.send(conn)
try:
resp = await req.send(conn)
except BaseException:
conn.close()
raise
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

try:
await resp.start(conn, read_until_eof)
except BaseException:
Expand Down
37 changes: 37 additions & 0 deletions tests/test_client_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ async def create_connection(req, traces=None):
# return self.transport, self.protocol
return mock.Mock()
session._connector._create_connection = create_connection
session._connector._release = mock.Mock()

with pytest.raises(aiohttp.ClientOSError) as ctx:
await session.request('get', 'http://example.com')
Expand All @@ -379,6 +380,42 @@ async def create_connection(req, traces=None):
assert e.strerror == err.strerror


async def test_close_conn_on_error(create_session):
class UnexpectedException(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pass

err = UnexpectedException("permission error")
req = mock.Mock()
req_factory = mock.Mock(return_value=req)
req.send = mock.Mock(side_effect=err)
session = create_session(request_class=req_factory)

connections = []
original_connect = session._connector.connect

async def connect(req, traces=None):
conn = await original_connect(req, traces=traces)
connections.append(conn)
return conn

async def create_connection(req, traces=None):
# return self.transport, self.protocol
conn = mock.Mock()
return conn

session._connector.connect = connect
session._connector._create_connection = create_connection
session._connector._release = mock.Mock()

with pytest.raises(UnexpectedException):
await session.request('get', 'http://example.com')

# normally called during garbage collection. triggers an exception
# if the connection wasn't already closed
for c in connections:
c.__del__()


async def test_cookie_jar_usage(loop, aiohttp_client):
req_url = None

Expand Down