-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-28369: Enhance transport socket check in add_reader/writer #4365
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
Changes from 1 commit
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 |
---|---|---|
|
@@ -246,8 +246,16 @@ def _accept_connection2(self, protocol_factory, conn, extra, | |
self.call_exception_handler(context) | ||
|
||
def _ensure_fd_no_transport(self, fd): | ||
fileno = fd | ||
if not isinstance(fileno, int): | ||
try: | ||
fileno = int(fileno.fileno()) | ||
except (AttributeError, TypeError, ValueError): | ||
# This code matches `selectors._fileobj_to_fd` function. | ||
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. Please avoid backticks -- without them the comment is still pretty clean. 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. Removed, but this is a very personal preference -- for me it was easier to read that comment with backticks/quotes. |
||
raise ValueError("Invalid file object: " | ||
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. Not covered by tests 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. Added a functional test for this. |
||
"{!r}".format(fileno)) from None | ||
try: | ||
transport = self._transports[fd] | ||
transport = self._transports[fileno] | ||
except KeyError: | ||
pass | ||
else: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -361,6 +361,13 @@ def assert_writer(self, fd, callback, *args): | |
handle._args, args) | ||
|
||
def _ensure_fd_no_transport(self, fd): | ||
if not isinstance(fd, int): | ||
try: | ||
fd = int(fd.fileno()) | ||
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. Not covered |
||
except (AttributeError, TypeError, ValueError): | ||
# This code matches `selectors._fileobj_to_fd` function. | ||
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. Avoid backticks. 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. Why? 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. @1st1 I hate any complex markup in comments, |
||
raise ValueError("Invalid file object: " | ||
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. Not covered. 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. This won't be covered as it's test_utils' test loop. There's no point in covering it. 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. Maybe let's add |
||
"{!r}".format(fd)) from None | ||
try: | ||
transport = self._transports[fd] | ||
except KeyError: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Enhance add_reader/writer check that socket is not used by some transport. | ||
Before, only cases when add_reader/writer were called with an int FD were | ||
supported. Now the check is implemented correctly for all file-like | ||
objects. |
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.
Please not make a new
fileno
local variable but reusefd
in the rest of function -- likeselectors._fileobj_to_fd
does.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.
fd
is used later in this function to format a better error message. This is not a copy/paste ofselectors._fileobj_to_fd
.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.
Got it, sorry.