bpo-30808: Use _Py_atomic API for concurrency-sensitive signal state

[pitrou]

Based on PR #2415.

[pitrou]

To be frank I'm not sure if _Py_atomic_int is necessary for Handlers[...].tripped, since is_tripped accesses should now act as full memory fences. But I guess better safe than sorry.

[pitrou]

@Haypo any concerns here?

[vstinner]

@Haypo any concerns here?

I disagree with the "trivial" label. You need to add a bpo number. I don't understand the rationale of using an atomic type, can you please elaborate?

[vstinner]

Based on PR #2415.

I don't see a direct link with this PR. Is it required to fix PR #2415?

[pitrou]

"Based on" just means it contains the changes from that PR. Now that it's merged, it doesn't matter anymore.

[pitrou]

I disagree with the "trivial" label.

There is no "trivial" label :-)

I don't understand the rationale of using an atomic type, can you please elaborate?

"Atomic" types are needed for proper interaction between concurrent running portions of code that don't use locks. Imagine trip_signal() in one thread and PyErr_CheckSignals() in another thread, for example.

We already use them in ceval.c for similar purposes.

[vstinner]

Ah, thanks for the link to http://bugs.python.org/issue30808

[pitrou]

@cf-natali, I'd like your advice here.

[Paxxi]

I'm thinking a lock would be better here, atomics are great for a single flag variable or ref count. In this case there's an array of flags and another thread might change tripped before it's set to 0 and losing a signal or several?

I haven't dug into how this code is called so it may be a non-issue.

[pitrou]

Unfortunately, locks can't be used in signal-handling code.

[Paxxi]

ahh right. Then this is a clear improvement over the current situation, 👍