-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Fixed Inappropriate Logical Expression #7956
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Removed break statement inside the 'finally' block of a try-catch statement otherwise it can suppress unhandled exceptions. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -602,8 +602,6 @@ async def start(self) -> None: | |
self._keepalive_handle = loop.call_at( | ||
now + keepalive_timeout, self._process_keepalive | ||
) | ||
else: | ||
break | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, in most cases, it'll probably be a KeyboardInterrupt, which may or may not lead to the program exiting..
Seems fine with me. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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: | ||
|
There was a problem hiding this comment.
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..