-
-
Notifications
You must be signed in to change notification settings - Fork 19
Update alter-var-root to return new val, updated docs #1171
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
Conversation
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.
Functionally LGTM. Had some minor tweaks to text but ready to approve after that.
Co-authored-by: Chris Rink <[email protected]>
Co-authored-by: Chris Rink <[email protected]>
Co-authored-by: Chris Rink <[email protected]>
|
Hi @chrisrink10, before you mrege, let me add some instrumentation to the intermittent windows test to try to better understand the problem with the timeout expiring earlier than set. |
I've added logging for the timer values in case it helps with the investigation. I can't see why the deref could return before the timeout has expired. I've also had a quick look at the thread pool executor and it appears the timeout is eventually passed to a threading.conditional, so that should have worked as expected at that level I think (i.e. the conditional shouldn't have returned before the timeout has expired). Python future conditional wait: https://github.com/python/cpython/blob/035f512046337e64a018d11fdaa3b21758625291/Lib/concurrent/futures/_base.py#L443 Python conditional wait implementation on win: https://github.com/python/cpython/blob/035f512046337e64a018d11fdaa3b21758625291/Python/condvar.h#L171 WaitForSingleObjectEx on win: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobjectex |
Hi,
can you please review change to have have
alter-var-rootto return the new value. It addresses #1166.I've also updated various part of the documentation to note the effect of direct linking on this fn. Please feel free to adjust partially or completely.
I've also added test cases for the
alter-var-rootfor the new change plus to demo the direct linking behavior.Thanks