Skip to content

bpo-23819: asyncio: Replace AssertionError with proper excs #29894

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

Merged
asvetlov merged 5 commits into from
Dec 6, 2021
Merged

bpo-23819: asyncio: Replace AssertionError with proper excs #29894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4fa65c
bpo-23819: Replace AssertionError with proper excs
kumaraditya303 Nov 30, 2021
3d95ee3
code review
kumaraditya303 Dec 2, 2021
06f0b39
code review
kumaraditya303 Dec 3, 2021
97f82d4
convert to TypeError
kumaraditya303 Dec 4, 2021
67a68ea
add to call_later
kumaraditya303 Dec 5, 2021
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: 4 additions & 0 deletions Lib/asyncio/base_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,8 @@ def call_later(self, delay, callback, *args, context=None):
Any positional arguments after the callback will be passed to
the callback when it is called.
"""
if delay is None:
raise TypeError('delay must not be None')
Comment on lines +709 to +710

@serhiy-storchaka serhiy-storchaka Dec 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is redundant. A TypeError will be raised at the next line, by self.time() + delay.

@asvetlov asvetlov Dec 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My thought when I suggested the change was: better to explicitly warn about None argument than a little cryptic unsupported operand type(s) for +: 'NoneType' and 'int'.
I believe not only exception type but also a proper message can help users to avoid silly mistakes.

@serhiy-storchaka serhiy-storchaka Dec 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is None special for this argument? I do not know any precedents of checkinjg only for None if other types can be invalid too. It looks like an antipattern to me. We either check for allowed types, or do not check either and let an error be raised later when perform an unsupported operation.

timer = self.call_at(self.time() + delay, callback, *args,
context=context)
if timer._source_traceback:
Expand All @@ -717,6 +719,8 @@ def call_at(self, when, callback, *args, context=None):

Absolute time corresponds to the event loop's time() method.
"""
if when is None:
raise TypeError("when cannot be None")
Comment on lines +722 to +723

@serhiy-storchaka serhiy-storchaka Dec 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why test only for None? What about other invalid types?

@asvetlov asvetlov Dec 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same as above: provide descriptive error message for obvious mistakes, raise default errors for all other combinations.

self._check_closed()
if self._debug:
self._check_thread()
Expand Down
7 changes: 4 additions & 3 deletions Lib/asyncio/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class TimerHandle(Handle):
__slots__ = ['_scheduled', '_when']

def __init__(self, when, callback, args, loop, context=None):
assert when is not None
Comment thread
asvetlov marked this conversation as resolved.
super().__init__(callback, args, loop, context)
if self._source_traceback:
del self._source_traceback[-1]
Expand Down Expand Up @@ -661,7 +660,8 @@ def get_event_loop(self):
def set_event_loop(self, loop):
"""Set the event loop."""
self._local._set_called = True
assert loop is None or isinstance(loop, AbstractEventLoop)
if loop is not None and not isinstance(loop, AbstractEventLoop):
raise TypeError(f"loop must be an instance of AbstractEventLoop or None, not '{type(loop).__name__}'")
self._local._loop = loop

def new_event_loop(self):
Expand Down Expand Up @@ -745,7 +745,8 @@ def set_event_loop_policy(policy):

If policy is None, the default policy is restored."""
global _event_loop_policy
assert policy is None or isinstance(policy, AbstractEventLoopPolicy)
if policy is not None and not isinstance(policy, AbstractEventLoopPolicy):
raise TypeError(f"policy must be an instance of AbstractEventLoopPolicy or None, not '{type(policy).__name__}'")
_event_loop_policy = policy


Expand Down
4 changes: 4 additions & 0 deletions Lib/test/test_asyncio/test_base_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,8 @@ def cb():
self.assertIsInstance(h, asyncio.TimerHandle)
self.assertIn(h, self.loop._scheduled)
self.assertNotIn(h, self.loop._ready)
with self.assertRaises(TypeError, msg="delay must not be None"):
self.loop.call_later(None, cb)

def test_call_later_negative_delays(self):
calls = []
Expand Down Expand Up @@ -286,6 +288,8 @@ def cb():
# tolerate a difference of +800 ms because some Python buildbots
# are really slow
self.assertLessEqual(dt, 0.9, dt)
with self.assertRaises(TypeError, msg="when cannot be None"):
self.loop.call_at(None, cb)

def check_thread(self, loop, debug):
def cb():
Expand Down
8 changes: 2 additions & 6 deletions Lib/test/test_asyncio/test_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2322,10 +2322,6 @@ def callback(*args):
self.assertIsNone(h._callback)
self.assertIsNone(h._args)

# when cannot be None
self.assertRaises(AssertionError,
asyncio.TimerHandle, None, callback, args,
self.loop)

def test_timer_repr(self):
self.loop.get_debug.return_value = False
Expand Down Expand Up @@ -2592,7 +2588,7 @@ def test_set_event_loop(self):
policy = asyncio.DefaultEventLoopPolicy()
old_loop = policy.get_event_loop()

self.assertRaises(AssertionError, policy.set_event_loop, object())
self.assertRaises(TypeError, policy.set_event_loop, object())

loop = policy.new_event_loop()
policy.set_event_loop(loop)
Expand All @@ -2608,7 +2604,7 @@ def test_get_event_loop_policy(self):

def test_set_event_loop_policy(self):
self.assertRaises(
AssertionError, asyncio.set_event_loop_policy, object())
TypeError, asyncio.set_event_loop_policy, object())

old_policy = asyncio.get_event_loop_policy()

Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2021-12-02-14-37-30.bpo-23819.An6vkT.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replaced asserts with exceptions in asyncio, patch by Kumar Aditya.