Skip to content

py38: cannot set_result more than once #28700

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
joshuarli merged 6 commits into from
Sep 21, 2021
Merged

py38: cannot set_result more than once #28700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9280785
you can't set_result more than once in python 3.8
joshuarli Sep 9, 2021
f30c04d
doesn't stall for me
joshuarli Sep 9, 2021
61ad105
keep existing behavior
joshuarli Sep 20, 2021
1913b01
assert that time isn't overwritten
joshuarli Sep 20, 2021
d4dd8e2
guard with version check
joshuarli Sep 21, 2021
def9735
lol
joshuarli Sep 21, 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
8 changes: 4 additions & 4 deletions src/sentry/utils/concurrent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,10 @@ def cancel(self, *args, **kwargs):

def set_result(self, *args, **kwargs):
with self._condition:
# This method always overwrites the result, so we always overwrite
# the timing, even if another timing was already recorded.
self.__timing[1] = time()
return super().set_result(*args, **kwargs)
_time = time()
result = super().set_result(*args, **kwargs)
Copy link
Member Author

@joshuarli joshuarli Sep 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Reordered things to not overwrite the timings in case set_result raises.

self.__timing[1] = _time
return result

def set_exception(self, *args, **kwargs):
with self._condition:
Expand Down
27 changes: 19 additions & 8 deletions tests/sentry/utils/test_concurrent.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
)


@pytest.mark.skipif(sys.version_info[0] == 3, reason="TODO(python3): stalls on python3")
def test_execute():
assert execute(_thread.get_ident).result() != _thread.get_ident()

Expand Down Expand Up @@ -108,9 +107,25 @@ def test_timed_future_success():
future.set_result(None)
assert future.get_timing() == (1.0, 2.0)

with timestamp(3.0):
future.set_result(None)
assert future.get_timing() == (1.0, 3.0)

@pytest.mark.skipif(sys.version_info[:2] < (3, 8), reason="doesn't apply to this python version")
def test_time_is_not_overwritten_if_fail_to_set_result():
future = TimedFuture()

with timestamp(1.0):
future.set_running_or_notify_cancel()
future.set_result(1)
assert future.get_timing() == (1.0, 1.0)

from concurrent.futures import InvalidStateError

with timestamp(2.0):
try:
future.set_result(1)
except InvalidStateError:
pass
# If set_result fails, the time shouldn't be overwritten.
assert future.get_timing() == (1.0, 1.0)


def test_timed_future_error():
Expand All @@ -125,10 +140,6 @@ def test_timed_future_error():
future.set_exception(None)
assert future.get_timing() == (1.0, 2.0)

with timestamp(3.0):
future.set_exception(None)
assert future.get_timing() == (1.0, 3.0)


def test_timed_future_cancel():
future = TimedFuture()
Expand Down