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

Add thread ID support to std.os.Thread (fixes #1316) #1330

Merged
merged 3 commits into from
Aug 6, 2018

Conversation

mdsteele
Copy link
Contributor

@mdsteele mdsteele commented Aug 4, 2018

This provides a cross-platform way to both (1) get a unique (opaque) ID for the current thread (using std.os.Thread.currentId()), and (2) given a std.os.Thread object, get the unique ID that that thread would get if it called std.os.Thread.currentId(). Note that this second thing is something the proposed getThreadId solution in #1316 using threadlocal can't do.

What do you think?

@mdsteele
Copy link
Contributor Author

mdsteele commented Aug 4, 2018

Hmm, looks like it works on Mac (which is what I wrote and tested this on) and fails on Linux and Windows. So I will have to dig into this.

kristate added a commit to kristate/zig that referenced this pull request Aug 4, 2018
@kristate
Copy link
Contributor

kristate commented Aug 4, 2018

@mdsteele I sent you a pull request.
mdsteele#1

Copy link
Contributor

@kristate kristate left a comment

Choose a reason for hiding this comment

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

Almost there...

std/os/index.zig Outdated
@@ -2516,26 +2516,56 @@ pub const Thread = struct {
data: Data,

pub const use_pthreads = is_posix and builtin.link_libc;

/// An opaque type representing a kernel thread ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove/update this comment because it does not match the code now.

std/os/test.zig Outdated
}

test "std.os.Thread.currentId" {
var threadCurrentId: ?os.Thread.Id = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

os.Thread.currentId() should always return os.Thread.Id, so no need to make it optional.

fn testThreadIdFn(threadId: *?os.Thread.Id) void {
to
fn testThreadIdFn(threadId: *os.Thread.Id) void {

std/os/test.zig Outdated
const thread = try os.spawnThread(&threadCurrentId, testThreadIdFn);
const threadId = thread.id();
thread.wait();
assert(threadCurrentId == threadId);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you fix the above, this should work.

@andrewrk
Copy link
Member

andrewrk commented Aug 6, 2018

I fixed it on linux in a local branch. Just needed to use gettid instead of getpid.

For windows, I think this is the problem:

GetCurrentThread

Retrieves a pseudo handle for the calling thread.
A pseudo handle is a special constant that is interpreted as the current thread handle. The calling thread can use this handle to specify itself whenever a thread handle is required.

So the result of Thread.getCurrentId using this API is not comparable with the handle that we get from CreateThread.

@mdsteele
Copy link
Contributor Author

mdsteele commented Aug 6, 2018

I fixed it on linux in a local branch. Just needed to use gettid instead of getpid.

Whoops, duh. I think I saw that the handle variable for Linux was called pid, and was like "Sure, I want getpid()" without really thinking about it. /-:

For windows, I think this is the problem:

GetCurrentThread

Retrieves a pseudo handle for the calling thread. A pseudo handle is a special constant that is interpreted as the current thread handle. The calling thread can use this handle to specify itself whenever a thread handle is required.

So the result of Thread.getCurrentId using this API is not comparable with the handle that we get from CreateThread.

Whoops again. I should probably be reading the documentation of these functions I'm calling, huh?

Speaking of said documentation, I see it also says this:

The function cannot be used by one thread to create a handle that can be used by other threads to refer to the first thread. The handle is always interpreted as referring to the thread that is using it. A thread can create a "real" handle to itself that can be used by other threads, or inherited by other processes, by specifying the pseudo handle as the source handle in a call to the DuplicateHandle function.

Sounds promising. I'll look into it.

@andrewrk andrewrk merged commit 7a2401e into ziglang:master Aug 6, 2018
@andrewrk
Copy link
Member

andrewrk commented Aug 6, 2018

I believe I have the tests passing on all platforms: Here's what I did:

  • Introduce Thread.Handle which is distinct from Thread.Id. On POSIX they are the same; on Windows they are different. Both types are documented to be "either a pointer or an integer".
  • rename currentId to getCurrentId to communicate that it does non-trivial work, and remove the TODO comment in favor of documenting it to always make a syscall. It returns Id rather than Handle.
  • rename id() to handle(). So now it's clearer that on some platforms you cannot compare this value with getCurrentId().
  • The test tests different things on different platforms.

I thought about DuplicateHandle but I don't think it accomplishes anything for us. It still wouldn't let us compare the original handle with the new handle, since the duplicate one would be a different address. Plus you'd introduce the ability of getCurrentId to fail, and require the caller to clean up resources.

I don't think this is where the API will stay. It's pretty messy and not clear where the cross platform abstraction ends and begins, however, let's focus on your allocator use case and make sure that is solved appropriately, and then we can revisit the std lib API before 1.0.0.

If I understand correctly, what you're now going to do is use this as a way to detect which thread is calling into the allocator, using Thread.getCurrentId(), and you can use this until we have a better solution such as thread local variables (#924). Is that right?

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.

3 participants