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

Jump to bottom

Fixed Inappropriate Logical Expression #7956

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixed Inappropriate Logical Expression #7956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79050be
Fixed Inappropriate Logical Expression
fazledyn-or Dec 11, 2023
9f8b689
Added self to CHANGES & CONTIBUTORS list
fazledyn-or Dec 11, 2023
64d8263
Update web_protocol.py
Dreamsorcerer Jan 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/7956.bugfix
View file
Open in desktop
@@ -0,0 +1 @@
Removed break statement inside the 'finally' block of a try-catch statement otherwise it can suppress unhandled exceptions.
Copy link
Member

Choose a reason for hiding this comment

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

FWIW a changelog entry should explain the high-level effect for the end-users. Throwing internal implementation details at them doesn't seem useful..

1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
View file
Open in desktop
Expand Up @@ -50,6 +50,7 @@ Arie Bovenberg
Arseny Timoniq
Artem Yushkovskiy
Arthur Darcet
Ataf Fazledin Ahamed
Austin Scola
Ben Bader
Ben Greiner
Expand Down
5 changes: 3 additions & 2 deletions aiohttp/web_protocol.py
View file
Open in desktop
Expand Up @@ -589,6 +589,9 @@ async def start(self) -> None:
except Exception as exc:
self.log_exception("Unhandled exception", exc_info=exc)
self.force_close()
except BaseException:
self.force_close()
raise
finally:
if self.transport is None and resp is not None:
self.log_debug("Ignored premature client disconnection.")
Expand All @@ -602,8 +605,6 @@ async def start(self) -> None:
self._keepalive_handle = loop.call_at(
now + keepalive_timeout, self._process_keepalive
)
else:
break
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, I'm just trying to figure out if the transport.close() below should be run in the case of a BaseException that is uncaught..
@bdraco Do you think it's fine like this?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a risk of the transport not getting closed on BaseException. I'm not sure what that exception would be though.

maybe something like...

diff --git a/aiohttp/web_protocol.py b/aiohttp/web_protocol.py
index 3cefd5df..f2dcf5aa 100644
--- a/aiohttp/web_protocol.py
+++ b/aiohttp/web_protocol.py
@@ -589,6 +589,9 @@ class RequestHandler(BaseProtocol):
             except Exception as exc:
                 self.log_exception("Unhandled exception", exc_info=exc)
                 self.force_close()
+            except BaseException:
+                self.force_close()
+                raise
             finally:
                 if self.transport is None and resp is not None:
                     self.log_debug("Ignored premature client disconnection.")

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what that exception would be though.

Well, in most cases, it'll probably be a KeyboardInterrupt, which may or may not lead to the program exiting..

maybe something like...

Seems fine with me.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, the tests don't even complete, so the current change doesn't seem correct. Maybe with the proposed change it will work correctly..

Copy link
Member

Choose a reason for hiding this comment

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

Tests are still not completing. @fazledyn-or Any interest in finishing this PR?

Copy link
Author

Choose a reason for hiding this comment

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

Tests are still not completing. @fazledyn-or Any interest in finishing this PR?

It's been long since I had worked on it, and yeah- I'll work toward finishing the PR. However, I don't understand the reason behind introducing an empty raise.

Copy link
Member

Choose a reason for hiding this comment

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

I think in the original code if a BaseException happened, it could trigger the break, which then leads to closing the transport outside the exception handler. We shouldn't be suppressing a BaseException though, so to be safe we want to force close on a BaseException and then reraise it.

Looking over that code again, it feels like that block of logic should maybe not be in the finally block...


# remove handler, close transport if no handlers left
if not self._force_close:
Expand Down