Skip to content

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

Merged
pitrou merged 6 commits into
python:masterfrom
pitrou:signal_pyatomic
Jul 17, 2017
Merged

bpo-30808: Use _Py_atomic API for concurrency-sensitive signal state#2417
pitrou merged 6 commits into
python:masterfrom
pitrou:signal_pyatomic

Conversation

@pitrou

@pitrou pitrou commented Jun 26, 2017

Copy link
Copy Markdown
Member

Based on PR #2415.

pitrou added 4 commits June 26, 2017 19:16
@pitrou
Improve signal delivery
0a68ec4 
Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions.
@pitrou
Remove unused function
161cd38
@pitrou
Improve comments
b577db5
@pitrou
Use _Py_atomic API for concurrency-sensitive signal state
1b6b61d
@pitrou

pitrou commented Jun 26, 2017

Copy link
Copy Markdown
Member Author

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 pitrou force-pushed the signal_pyatomic branch from a0123f2 to 8c21768 Compare June 29, 2017 18:05
@pitrou pitrou changed the title [WIP] Use _Py_atomic API for concurrency-sensitive signal state Use _Py_atomic API for concurrency-sensitive signal state Jun 29, 2017
@pitrou pitrou added the type-feature A feature request or enhancement label Jun 29, 2017
@pitrou

pitrou commented Jun 29, 2017

Copy link
Copy Markdown
Member Author

@Haypo any concerns here?

@vstinner

vstinner commented Jun 29, 2017

Copy link
Copy Markdown
Member

@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

vstinner commented Jun 29, 2017

Copy link
Copy Markdown
Member

Based on PR #2415.

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

@pitrou

pitrou commented Jun 29, 2017

Copy link
Copy Markdown
Member Author

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

@pitrou

pitrou commented Jun 29, 2017

Copy link
Copy Markdown
Member Author

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.

@pitrou pitrou changed the title Use _Py_atomic API for concurrency-sensitive signal state bpo-30808: Use _Py_atomic API for concurrency-sensitive signal state Jun 29, 2017
@vstinner

vstinner commented Jun 29, 2017

Copy link
Copy Markdown
Member

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

@pitrou

pitrou commented Jun 30, 2017

Copy link
Copy Markdown
Member Author

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

@pitrou pitrou mentioned this pull request Jul 8, 2017
Merged
@Paxxi

Paxxi commented Jul 9, 2017

Copy link
Copy Markdown
Contributor

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

pitrou commented Jul 9, 2017

Copy link
Copy Markdown
Member Author

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

@Paxxi

Paxxi commented Jul 10, 2017

Copy link
Copy Markdown
Contributor

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

@pitrou pitrou merged commit 2c8a5e4 into python:master Jul 17, 2017
@pitrou pitrou deleted the signal_pyatomic branch July 17, 2017 10:25
pitrou added a commit to pitrou/cpython that referenced this pull request Aug 6, 2017
@pitrou
[3.6] bpo-30808: Use _Py_atomic API for concurrency-sensitive signal …
e4f02b6 
…state (pythonGH-2417)

* Improve signal delivery

Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions.

* Remove unused function

* Improve comments

* Use _Py_atomic API for concurrency-sensitive signal state

* Add blurb
(cherry picked from commit 2c8a5e4)
pitrou added a commit that referenced this pull request Aug 6, 2017
@pitrou
[3.6] bpo-30808: Use _Py_atomic API for concurrency-sensitive signal …
57e836c 
…state (GH-2417) (#3007)

* Improve signal delivery

Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions.

* Remove unused function

* Improve comments

* Use _Py_atomic API for concurrency-sensitive signal state

* Add blurb
(cherry picked from commit 2c8a5e4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pitrou @vstinner @Paxxi @the-knights-who-say-ni