[issue30038] Race condition in how trip_signal writes to wakeup fd

Changes by Nathaniel Smith : -- resolution: -> fixed stage: -> resolved status: open -> closed ___ Python tracker ___

[issue30038] Race condition in how trip_signal writes to wakeup fd

Antoine Pitrou added the comment: Why is this PR not closed? -- ___ Python tracker ___ ___ Python-bugs-list

[issue30038] Race condition in how trip_signal writes to wakeup fd

Ned Deily added the comment: New changeset 0a794a3256b24ccf57b18ec9964f2367ac1f3d30 by Ned Deily in branch '3.6': bpo-30038: add Misc/NEWS entry. https://github.com/python/cpython/commit/0a794a3256b24ccf57b18ec9964f2367ac1f3d30 -- ___ Python

[issue30038] Race condition in how trip_signal writes to wakeup fd

Changes by Ned Deily : -- pull_requests: +2309 ___ Python tracker ___ ___ Python-bugs-list

[issue30038] Race condition in how trip_signal writes to wakeup fd

Ned Deily added the comment: New changeset 6a98a04e21fc3209b2170c3930ce4488e89680e3 by Ned Deily in branch 'master': bpo-30038: add Misc/NEWS entry. (#2258) https://github.com/python/cpython/commit/6a98a04e21fc3209b2170c3930ce4488e89680e3 -- ___

[issue30038] Race condition in how trip_signal writes to wakeup fd

Changes by Ned Deily : -- pull_requests: +2308 ___ Python tracker ___ ___ Python-bugs-list

[issue30038] Race condition in how trip_signal writes to wakeup fd

Changes by Ned Deily : -- pull_requests: +2307 ___ Python tracker ___ ___ Python-bugs-list

[issue30038] Race condition in how trip_signal writes to wakeup fd

Ned Deily added the comment: Misc/NEWS entries for 3.6 and master for these changes? -- ___ Python tracker ___

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: New changeset 0b7629cd846e3e160ac07cafc4e97644024f11b8 by Victor Stinner in branch '3.6': bpo-30038: fix race condition in signal delivery + wakeup fd (#1082) (#2075) https://github.com/python/cpython/commit/0b7629cd846e3e160ac07cafc4e97644024f11b8

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: Ned Deily and Yury Selivanov are in favor of a backport, thank you for looking at this one guys. So I created the PR 2075. I will merge it once the CI pass. -- ___ Python tracker

[issue30038] Race condition in how trip_signal writes to wakeup fd

Changes by STINNER Victor : -- pull_requests: +2138 ___ Python tracker ___ ___

[issue30038] Race condition in how trip_signal writes to wakeup fd

Yury Selivanov added the comment: > It sounds like a bug to me, and thus suitable to backport, but it certainly > would be worth getting other opinions since this is a tricky and critical > area. I think it's safe to merge this in 3.6. -- ___

[issue30038] Race condition in how trip_signal writes to wakeup fd

Ned Deily added the comment: It sounds like a bug to me, and thus suitable to backport, but it certainly would be worth getting other opinions since this is a tricky and critical area. Antoine, Steve, others: thoughts? -- nosy: +pitrou, steve.dower

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: But, by that definition, like... every change is backwards incompatible. I'm pretty confident that no-one was relying on this race condition. I can't even figure out what that would mean. -- ___ Python tracker

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: > I think you mean it's backwards *compatible*? I mean incompatible since it changes the behaviour in a subtle way. -- ___ Python tracker

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: I think you mean it's backwards *compatible*? There's definitely no change in APIs or ABIs or anything, all that changes is the order of some statements inside Python's signal handler. (We used to we write to the magic wakeup fd and then set a flag saying a

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: @Ned Deily: Hey, Nathaniel wants to backport the commit 4ae01496971624c75080431806ed1c08e00f22c7 which changes how the C signal handler of CPython. There is a low risk of regression, it can be seen as a backward incompatible change. I'm not super excited to

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: I guess now would be a good time to decide whether this should be backported to 3.6, with 3.6.2 coming out in a few days :-). (Or if not, then it can probably be closed?) -- ___ Python tracker

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: > (BTW do you happen to know any tricks to force CPython to do an immediate > PyErr_CheckSignals on Windows?) Never mind on this... it looks like calling repr() on any object is sufficient. -- ___ Python tracker

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: > While I suggest you to *not* use an event loop (wakeup fd pipe/socket handle > with select) and signal.signal(), you are true that there is a race condition > if you use select() with signal.signal() so I merged your change. Unfortunately this is the only

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: While I suggest you to *not* use an event loop (wakeup fd pipe/socket handle with select) and signal.signal(), you are true that there is a race condition if you use select() with signal.signal() so I merged your change. Do you consider that it's worth it to

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: New changeset 4ae01496971624c75080431806ed1c08e00f22c7 by Victor Stinner (Nathaniel J. Smith) in branch 'master': bpo-30038: fix race condition in signal delivery + wakeup fd (#1082)

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: The attached script wakeup-fd-racer.py fails consistently for me using cpython 3.6.0 on my windows 10 vm: > python wakeup-fd-racer.py Attempt 0: start Attempt 0: FAILED, took 10.0160076 seconds select_calls = 2 (It may help that the VM only has 1

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: I know that it can be very difficult to write such test, but can you please try to write a script trying to reproduce the describe the race condition? Later, we can run the script in a test to check for non-regression. --

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: "I think the idea in c13ef6664998 wasn't so much that we wanted the wakeup fd to be written to first, as that the way the code was written back then, the presence of 'if (is_tripped) return;' meant that it wasn't getting written to *at all* in some cases.

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: If it helps, notice that the SetEvent(sigint_event) call used to wake up the main thread on windows is also performed unconditionally and after the call to Py_AddPendingEvent. From the point of view of twisted/tornado/trio, this is exactly the same as the

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: I think the idea in c13ef6664998 wasn't so much that we wanted the wakeup fd to be written to first, as that the way the code was written back then, the presence of 'if (is_tripped) return;' meant that it wasn't getting written to *at all* in some cases.

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: Previous changes in signal handling. It's the commit c13ef66649985025382c64f6af8e3b956411e05b of the issue #8407 which changed the order: became . I really *hate* having to think to these evil things which are signals and

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: Err, libuv obviously doesn't use a Python-level signal handler. I just meant to include them as another example of a library I checked that uses a self-pipe to handle signals but relies on out-of-band information to transmit what the actual signal is :-).

[issue30038] Race condition in how trip_signal writes to wakeup fd

Nathaniel Smith added the comment: Right. My claim would be that the PR I just submitted is the correct fix for bpo-21645 as well. The approach asyncio uses is very elegant, but unfortunately it assumes that the wakeup fd has infinite buffer, which isn't true. If enough signals or other

[issue30038] Race condition in how trip_signal writes to wakeup fd

STINNER Victor added the comment: Last time I had to make a major change related to signal handling, it was in the asyncio module because of a race conditon which occurred on FreeBSD. commit fe5649c7b7bf52147480d6b1124a3d8e3597aee3 Author: Victor Stinner Date: Thu

[issue30038] Race condition in how trip_signal writes to wakeup fd

Changes by Nathaniel Smith : -- pull_requests: +1226 ___ Python tracker ___ ___

[issue30038] Race condition in how trip_signal writes to wakeup fd

New submission from Nathaniel Smith: In trip_signal [1], the logic goes: 1) set the flag saying that this particular signal was tripped 2) write to the wakeup fd 3) set the global is_tripped flag saying "at least one signal was tripped", and do Py_AddPendingCall (which sets some global flags