Skip to content
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

Document BaseFuture semi-private methods. #225

Open
mdickinson opened this issue Oct 15, 2020 · 4 comments
Open

Document BaseFuture semi-private methods. #225

mdickinson opened this issue Oct 15, 2020 · 4 comments
Labels
component: documentation Issues related to developer and user documentation

Comments

@mdickinson
Copy link
Member

The semi-private methods of the BaseFuture class are depended on by the executor machinery. They're useful for unit-testing for projects using Traits Futures: if you want to test side-effects resulting from state changes to a Future class, it's useful to be able to simulate the results of a running background job.

Those methods currently appear to be private, via the usual leading underscore convention. We need to document that they're safe to use, and that backwards compatibility will be taken into account when making updates.

Specifically, the methods of interest are these ones:

def _task_started(self, none):
"""
Update state when the background task has started processing.
"""
if self._state == _INITIALIZED:
self._state = EXECUTING
elif self._state == _CANCELLING_BEFORE_STARTED:
self._state = _CANCELLING_AFTER_STARTED
else:
raise _StateTransitionError(
"Unexpected 'started' message in state {!r}".format(
self._state
)
)
def _task_returned(self, result):
"""
Update state when background task reports completing successfully.
"""
if self._state == EXECUTING:
self._cancel = None
self._result = result
self._state = COMPLETED
elif self._state == _CANCELLING_AFTER_STARTED:
self._state = CANCELLED
else:
raise _StateTransitionError(
"Unexpected 'returned' message in state {!r}".format(
self._state
)
)
def _task_raised(self, exception_info):
"""
Update state when the background task reports completing with an error.
"""
if self._state == EXECUTING:
self._cancel = None
self._exception = exception_info
self._state = FAILED
elif self._state == _CANCELLING_AFTER_STARTED:
self._state = CANCELLED
else:
raise _StateTransitionError(
"Unexpected 'raised' message in state {!r}".format(self._state)
)
def _user_cancelled(self):
"""
Update state when the user requests cancellation.
A future in ``WAITING`` or ``EXECUTING`` state moves to ``CANCELLING``
state.
"""
if self._state == _INITIALIZED:
self._cancel()
self._cancel = None
self._state = _CANCELLING_BEFORE_STARTED
elif self._state == EXECUTING:
self._cancel()
self._cancel = None
self._state = _CANCELLING_AFTER_STARTED
else:
raise _StateTransitionError(
"Unexpected 'cancelled' message in state {!r}".format(
self._state
)
)
def _executor_initialized(self, cancel):
"""
Update state when the executor initializes the future.
Parameters
----------
cancel : callable
The callback to be called when the user requests cancellation.
The callback accepts no arguments, and has no return value.
"""
if self._state == _NOT_INITIALIZED:
self._cancel = cancel
self._state = _INITIALIZED
else:
raise _StateTransitionError(
"Unexpected initialization in state {!r}".format(self._state)
)

Note that in contrast, the _state, _cancel, _result and _exception attributes are genuinely private, and should not be depended on by users.

Looking at the methods above, there's a bit of cleanup to be done (e.g., the _cancel trait can get set to None multiple times, which is inelegant), but I think we can guarantee not to change the names of any of those methods without warning.

We may also possibly want to create a formal interface (likely inheriting from IFuture) encapsulating those methods.

@mdickinson
Copy link
Member Author

mdickinson commented Oct 15, 2020

Possible updates needed:

  • There may also be a case for an _executor_finalized method.
  • Can we rename the _dispatch_message method to (a) start with _task, and (b) be grammatically consistent with the other state-changing method names? (E.g., _task_sent_custom_message)

@mdickinson mdickinson added this to the Release 0.3.0 milestone Oct 15, 2020
@mdickinson mdickinson added the component: documentation Issues related to developer and user documentation label Mar 27, 2021
@kitchoi
Copy link

kitchoi commented Apr 14, 2021

In addition to documentation, I'd like to mention that _task_raised expects an argument returned by marshal_exception, which means marshal_exception might be included in the public API for the purpose of testing.

@mdickinson
Copy link
Member Author

mdickinson commented Apr 14, 2021

Thanks, yes; that's a good point. I've never been quite happy with the exception marshalling (for one thing, in Python 3, with tracebacks attached to the exception objects, we should have no need at all to marshal exceptions across threads), and it's a part of the codebase that's been pretty much untouched since version 0.0.0. There's room for a rethink there at some point before the 1.0.0 release. But yes, as things stand it really needs to be a part of the public (or at least the testing) API.

It may make sense to grow a traits_futures.testing.api package.

@mdickinson
Copy link
Member Author

Not in this release.

@mdickinson mdickinson removed this from the Release 0.3.0 milestone Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: documentation Issues related to developer and user documentation
Projects
None yet
Development

No branches or pull requests

2 participants