Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Jul 17, 2017
Merged

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

merged 6 commits into from
Jul 17, 2017

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jun 26, 2017

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
Copy link
Member Author

pitrou commented Jun 26, 2017

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
Copy link
Member Author

pitrou commented Jun 29, 2017

@Haypo any concerns here?

@vstinner
Copy link
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
Copy link
Member

Based on PR #2415.

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

@pitrou
Copy link
Member Author

pitrou commented Jun 29, 2017

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

@pitrou
Copy link
Member Author

pitrou commented Jun 29, 2017

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
Copy link
Member

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

@pitrou
Copy link
Member Author

pitrou commented Jun 30, 2017

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

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

Paxxi commented Jul 9, 2017

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
Copy link
Member Author

pitrou commented Jul 9, 2017

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

@Paxxi
Copy link
Contributor

Paxxi commented Jul 10, 2017

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