-
Notifications
You must be signed in to change notification settings - Fork 541
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
use a stack of self._fds_to_close to prevent double closes #481
Conversation
Co-authored-by: Thomas Grainger <[email protected]>
and make tests easier to write because the close order is deterministic and in the order that opens happen in this should also be a bit faster because list.append is faster than set.add and we skip a call to os_close(-1) and catching an OSError exception
|
||
async def test(): | ||
proc = await asyncio.create_subprocess_exec( | ||
sys.executable, "-c", "pass" |
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.
you don't actually need a preexec_fn to trigger the double close it just made it slow enough that the race was more detectable
pass | ||
|
||
fds_to_close = self._fds_to_close | ||
self._fds_to_close = None |
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 self._fds_to_close = None
is still needed?
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.
Ah yep I see 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 moved it up so that _close_after_spawn can't interfere with the errpipe_write fd
newfd = os_dup(io[1]) | ||
os_set_inheritable(newfd, True) | ||
self._close_after_spawn(newfd) | ||
io[2] = newfd | ||
io[2] = self._file_redirect_stdio(io[1]) |
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.
when reviewing all the calls to _close_after_spawn
to make sure a set wasn't needed, I found this duplicate code
uvloop/uvloop/handles/process.pyx
Lines 398 to 402 in 5926386
cdef _file_redirect_stdio(self, int fd): | |
fd = os_dup(fd) | |
os_set_inheritable(fd, True) | |
self._close_after_spawn(fd) | |
return fd |
@fantix can I get a review on this please? |
@@ -8,7 +8,7 @@ cdef class UVProcess(UVHandle): | |||
object _preexec_fn | |||
bint _restore_signals | |||
|
|||
set _fds_to_close | |||
list _fds_to_close |
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.
@fantix I think there's a proper list[int]
data type in cython for this but I don't know how to use it
@fantix thanks for the review! |
This release adds Python 3.11 support, updates bundled libuv to 1.43.0 and fixes a handful of issues. Changes ======= * Expose uv_loop_t pointer for integration with other C-extensions (#310) (by @pranavtbhat in b332eb8 for #310) * Support python 3.11+ (#473) (by @zeroday0619 in 8e42921 for #473) * Expose libuv uv_fs_event functionality (#474) (by @jensbjorgensen @fantix in 74d381e for #474) * Activate debug mode when `-X dev` is used (by @jack1142 in 637a77a) * Expose uv_version() for libuv API compatibility (#491) (by @fantix in 089f6cb for #491) * Fix loop.getaddrinfo() and tests (#495) (by @fantix in 598b16f for #495) * Bump to libuv 1.43.0 (by @fantix in 94e5e53) Fixes ===== * _TransProtPair is no longer defined in asyncio.events (by @jensbjorgensen in fae5f7f) * use a TypeVar for asyncio.BaseProtocol (#478) (by @graingert in 3aacb35 for #478) * Fix segfault in TimerHandle.when() after cleared (by @jensbjorgensen in c39afff for #469) * Avoid self._errpipe_write double close (#466) (by @graingert in 72140d7 for #466) * Fix typo in test (#456) (by @kianmeng in 033d52d for #456) * Fix potential infinite loop (#446) (by @kfur in ada43c0 for #446) * use a stack of self._fds_to_close to prevent double closes (#481) (by @graingert in 3214cf6 for #481) * Fix incorrect main thread id value forking from a thread (#453) (by @horpto @fantix in e7934c8 for #453) * create_subprocess_exec should treat env={} as empty environment (#439) (#454) (by @byllyfish in e04637e for #439) * Queue write only after processing all buffers (#445) (by @jakirkham @fantix in 9c6ecb6 for #445) * Drop Python 3.6 support for thread ident (by @fantix in 9c37930) * bugfix: write to another transport in resume_writing() fails (#498) (by @fantix in d2deffe for #498) Build ===== * Upgrade GitHub Actions (#477) (#480) (by @cclauss in fcbf422 for #477, 1008694 for #480) * typo `same as same` (by @YoSTEALTH in fedba80) * setup.py: allow to override extra_compile_args (#443) (by @giuliobenetti in a130375 for #443) * Drop hack in setup.py in finalize_options (492) (by @fantix in 2f1bc83 for #492) * Fix tests invocation on release CI worklow (#489) (by @ben9923 in d6a2b59 for #489) Documentation ============= * use asyncio.Runner loop_factory on 3.11+ (#472) (by @graingert in 31ba48c for #472) * Fix CI badge in docs, remove remaining Travis CI references from docs (by @Nothing4You in c6901a7) * Fix typo in README (by @monosans in 73d7253)
This release adds Python 3.11 support, updates bundled libuv to 1.43.0 and fixes a handful of issues. Changes ======= * Expose uv_loop_t pointer for integration with other C-extensions (#310) (by @pranavtbhat in b332eb8 for #310) * Support python 3.11+ (#473) (by @zeroday0619 in 8e42921 for #473) * Expose libuv uv_fs_event functionality (#474) (by @jensbjorgensen @fantix in 74d381e for #474) * Activate debug mode when `-X dev` is used (by @jack1142 in 637a77a) * Expose uv_version() for libuv API compatibility (#491) (by @fantix in 089f6cb for #491) * Fix loop.getaddrinfo() and tests (#495) (by @fantix in 598b16f for #495) * Bump to libuv 1.43.0 (by @fantix in 94e5e53) Fixes ===== * _TransProtPair is no longer defined in asyncio.events (by @jensbjorgensen in fae5f7f) * use a TypeVar for asyncio.BaseProtocol (#478) (by @graingert in 3aacb35 for #478) * Fix segfault in TimerHandle.when() after cleared (by @jensbjorgensen in c39afff for #469) * Avoid self._errpipe_write double close (#466) (by @graingert in 72140d7 for #466) * Fix typo in test (#456) (by @kianmeng in 033d52d for #456) * Fix potential infinite loop (#446) (by @kfur in ada43c0 for #446) * use a stack of self._fds_to_close to prevent double closes (#481) (by @graingert in 3214cf6 for #481) * Fix incorrect main thread id value forking from a thread (#453) (by @horpto @fantix in e7934c8 for #453) * create_subprocess_exec should treat env={} as empty environment (#439) (#454) (by @byllyfish in e04637e for #439) * Queue write only after processing all buffers (#445) (by @jakirkham @fantix in 9c6ecb6 for #445) * Drop Python 3.6 support for thread ident (by @fantix in 9c37930) * bugfix: write to another transport in resume_writing() fails (#498) (by @fantix in d2deffe for #498) Build ===== * Upgrade GitHub Actions (#477) (#480) (by @cclauss in fcbf422 for #477, 1008694 for #480) * typo `same as same` (by @YoSTEALTH in fedba80) * setup.py: allow to override extra_compile_args (#443) (by @giuliobenetti in a130375 for #443) * Drop hack in setup.py in finalize_options (492) (by @fantix in 2f1bc83 for #492) * Fix tests invocation on release CI worklow (#489) (by @ben9923 in d6a2b59 for #489) Documentation ============= * use asyncio.Runner loop_factory on 3.11+ (#472) (by @graingert in 31ba48c for #472) * Fix CI badge in docs, remove remaining Travis CI references from docs (by @Nothing4You in c6901a7) * Fix typo in README (by @monosans in 73d7253)
and make tests easier to write because the close order is deterministic
and in the order that opens happen in
this should also be a bit faster because
list.append
is fasterthan
set.add
and we skip a call toos_close(-1)
and catching anOSError
exception