Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
PythonFutureTask
#46Add
PythonFutureTask
#46Changes from 12 commits
c624e35
1d10a54
dfb1f6e
ebf0120
6552c96
86609ac
70f64df
0ce583f
5272bb9
6c06dc1
fadc96b
32b800c
e621984
d71faa3
e4b928d
24989fe
9a7491f
1e68b19
f6a8c29
514627d
72eb539
6e63140
7e0ad1b
161e015
e55efdd
25cb66c
8439f95
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think calling
Py_XDECREF(_handle);
is thread-safe (e.g. it might trigger the execution of a__del__
).Is it safe to grab the GIL here? Or should we delay the decref to a Python cleanup routine?
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.
I guess it is safe as long as the current thread is able to take the GIL, i.e. the "main" thread releases the GIL at some point. However, that doesn't mean it couldn't cause deadlocks depending on how the application makes use of that. Definitely a Python cleanup routine could make things safer but I can't think of a cleanup routine that would not require some complex Python mechanism, meaning it can't be implemented here alone, perhaps you already have something in mind? The only alternative I can think of, which is definitely not a good one, would be to implement a public
release()
method that the user is supposed to call where it is known that taking the GIL is safe and then change the destructor to print a warning/error ifrelease()
wasn't called.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.
Sorry, we already have a
release()
which has a different meaning (release the handle to the user). Replacerelease()
in my comment above withfree()
.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.
Do we need
_handle
aftersetResult()
orsetPythonException()
has been called? If not, maybe let them release the handle?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.
The
_handle
is theasyncio.Future
object, so if the user never got a reference to it viagetHandle()
/release()
before the C++ future completed and notified the Python future then the user will never be able to get it later if we release it insetResult()
/setPythonException()
. Therefore, I don't think this would work.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.
Agreed, that's probably our best option. It will take me until next week or the week after to get back to this, but I'll try to make it work.
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.
I think this one is still to be resolved. In any case,
Py_XDECREF
is not safe to call without the gil (so one shouldPyGILState_Ensure
before calling it.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.
I have now introduced a new singleton class
PythonFutureTaskCollector
which allows pushingPyObject*
when~PythonFutureTask()
runs instead of it requiring the GIL to decref those. The GIL and decref now happens whenPythonFutureTaskCollector::collect()
gets called, but that needs to be done by the application itself, which currently runs it during~ApplicationThread
.This is not an optimal solution, but I think it's probably the only one that guarantees objects are actually decrefed at some point, the alternative may be to register another
atexit
handler. A periodic call of that method could also improve things a bit during runtime but is not enough to ensure everything gets cleaned up before exiting, as we may always miss the last few destroyedPythonFutureTask
if the periodic call happens to be called one last time before some~PythonFutureTask
. Perhaps I'm overlooking some obvious better and guaranteed way to ensure we don't leave anyPyObject*
behind, if you can think of any please let me know @madsbk @wence- .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.
This kind of deferred collection with stop-the-world cleanup points is the way I (with others) solve morally the same problem in a distributed setting (where two processes have to agree to destruct objects collectively). This is kind of similar because we're needing to do collection at a point when we can guarantee synchronisation of some resource (in this case the gil + any other locks that might be required).
So I think this is fine. You might imagine that you'd want to also call this "collect" periodically during submission of tasks if that happens on the main thread that will hold the gil.
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.
I agree with that idea, what I'm not sure is who should be responsible for that. I can think of a few different approaches:
PythonFutureTask
, for example every time a new one is constructed;ApplicationThread
, perhaps duringprogressUntilSync()
;tornado.ioloop.PeriodicCallback
;PollingMode
.I think we should be doing one or more of those, but it's difficult to decide the best approach. Furthermore,
ApplicationThread
is meant to be an example only, how to actually do it probably depends on the actual application needs so we don't need to cover every possible alternative. We will need to find an appropriate solution to this for UCXX though, but that will happen in a follow-up PR.