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.

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

bpo-31596: Add an interface for pthread_getcpuclockid(3) #3756

Merged
merged 3 commits into from
Oct 5, 2017
Merged

bpo-31596: Add an interface for pthread_getcpuclockid(3) #3756

merged 3 commits into from
Oct 5, 2017

Conversation

pdox
Copy link

@pdox pdox commented Sep 26, 2017

Modeled on the interface for signal.pthread_kill

https://bugs.python.org/issue31596

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@pdox pdox changed the title Add an interface for pthread_getcpuclockid(3) bpo-31596: Add an interface for pthread_getcpuclockid(3) Sep 26, 2017
@pdox
Copy link
Author

pdox commented Sep 27, 2017

FYI, I signed CLA. Not sure why the label hasn't cleared.

@benjaminp
Copy link
Contributor

Thanks for the PR. Unfortunately, I think signal.pthread_kill is a rather broken API. At least in nptl, pthread_t is just a pointer, so pthread_kill may be used to dereference arbitrary memory.

>>> signal.pthread_kill(42, signal.SIGKILL)
Segmentation fault

It's a design flaw of threading that it exposes thread identifiers as integers at all. Even in C, pthreads specifies pthread_t to be opaque to the application. So, for this particular issue, I see two ways forward: 1) add a method to threading.Thread that returns the clock id 2) properly expose an opaque object for thread identifiers and have time.pthread_getcpuclockid take such an object.

@pdox
Copy link
Author

pdox commented Sep 29, 2017

Ah, we meet again. I should have known you wouldn't go easy on me.

I've decided to give option 2 a try. I have created https://bugs.python.org/issue31622 to track it, and will be sending a pull request shortly.

Copy link
Contributor

@benjaminp benjaminp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems others as not as concerned as me about identifiers, so I suppose we can move forward here. lgtm except for a few nits.


Return the *clk_id* of the thread-specific CPU-time clock for the specified *thread_id*.

Use :func:`threading.get_ident()` or the :attr:`~threading.Thread.ident`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No parens after the function name.

for *thread_id*.

.. warning::
Passing an invalid or expired thread_id may result in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*thread_id*

PyErr_SetFromErrno(PyExc_OSError);
return NULL;
}
return PyLong_FromLong((long)clk_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast is superfluous. If clockid_t ends up being something large, it's probably good to get the compiler warning for the implicit cast.

@pdox
Copy link
Author

pdox commented Oct 5, 2017

[ping]. did I address all your concerns?

@benjaminp benjaminp merged commit e14679c into python:master Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants