bpo-33238: Add InvalidStateError to concurrent.futures. by jhaydaman · Pull Request #7056

bpo-33238: Add InvalidStateError to concurrent.futures. by jhaydaman · Pull Request #7056 · python/cpython

Future.set_result and Future.set_exception now raise InvalidStateError
if the futures are not pending or running. This mirrors the behavior
of asyncio.Future, and prevents AssertionErrors in asyncio.wrap_future
when set_result is called multiple times.

https://bugs.python.org/issue33238

Future.set_result and Future.set_exception now raise InvalidStateError
if the futures are not pending or running. This mirrors the behavior
of asyncio.Future, and prevents AssertionErrors in asyncio.wrap_future
when set_result is called multiple times.

Choose a reason for hiding this comment

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

LGTM.

Please add a NEWS entry and update the docs.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

		Should only be used by Executor implementations and unit tests.
		"""
		with self._condition:
		if self._state in [CANCELLED, CANCELLED_AND_NOTIFIED, FINISHED]:

Choose a reason for hiding this comment

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

Use tuple instead of a list.
By this, the tuple is stored in code object.
List is recreated every time on the function execution.

Choose a reason for hiding this comment

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

I would normally use a tuple, but went with a list because that's the style the rest of the module used. Should I update the other methods in Future to use tuples?

		Should only be used by Executor implementations and unit tests.
		"""
		with self._condition:
		if self._state in [CANCELLED, CANCELLED_AND_NOTIFIED, FINISHED]:

Choose a reason for hiding this comment

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

list -> tuple

Choose a reason for hiding this comment

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

I'd actually use set here.

		f = create_future(state=PENDING)
		f.set_result(1)

		with self.assertRaises(futures.InvalidStateError):

Choose a reason for hiding this comment

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

Please use assertRaisesRegex to make sure that error text fits expectation.

		e = ValueError()
		f.set_exception(e)

		with self.assertRaises(futures.InvalidStateError):

Choose a reason for hiding this comment

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

assertRaisesRegex

Jason Haydaman added 2 commits

May 24, 2018 08:29

I have made the requested changes; please review again

Thanks for making the requested changes!

@1st1, @asvetlov: please review the changes made to this pull request.

1st1 approved these changes May 29, 2018

@asvetlov The PR looks fine to me now. Could you please take another look?

So will this end up in 3.7.0? I didn't plan for that to happen... but if so, at least the what's new entry must be added. @asvetlov

@1st1, it's your call at the moment. Unless it is backported, it won't be in 3.7.0. If you really think it should be, please make it happen right away so we can get it in 3.7.0b5 which is planned to be tagged very soon.

Well, it's a nice non-critical enhancement, so in my opinion it can wait till 3.8. By no means it should go to 3.7.1 and not to 3.7.0 though.

Ah, this is in master branch only and there's no backport to 3.7, my bad, sorry for the noise.

@jhaydaman Any reason you did not list InvalidStateError in __all__ at __init__.py ?

aisk mentioned this pull request

Jun 5, 2024