emscripten support?

[BurntSushi]

It sounds like this crate might not work on emscripten, which I guess doesn't have threads? I saw @badboy has a fix for this, but I was wondering if it could get put into thread-id proper? Are there any other issues at play here?

[badboy]

The thing is: this create actually works fine on Emscripten: it returns 0 as that is what is defined for pthread_self in Emscripten's pthread stub.
The problem is that further up the line 0 is assumed invalid for a thread id, triggering some other code path, leading to a borrow failure at runtime.
That's also why I never submitted my fix upstream: While it fixes this particular problem in regex (and thread_local) it is not a real bug by itself.

[BurntSushi]

@badboy Ah I see, I misunderstood that. Thank you for explaining! I guess this means this should be filed against thread_local instead? cc @Amanieu

[badboy]

Pretty much, though I can't fully retrace the steps right now, but it seems it was a design decision to make 0 invalid here: https://github.com/Amanieu/thread_local-rs/blob/master/src/lib.rs#L112-L114u

[Amanieu]

Hmm, this is a bit tricky: I do rely on thread IDs being non-zero, even though this crate doesn't strictly guarantee it.

One workaround would be to use the address of a thread-local variable, which is guaranteed to be non-null:

pub fn thread_id() -> usize {
    thread_local!(static KEY: u8 = unsafe { mem::uninitialized() });
    KEY.with(|x| x as *const _ as usize)
}

The downside is that this generates worse code than simply calling pthread_self directly (pthread_self only contains 2 instructions, including the ret).

[ruuda]

It is not a very intrusive change, but I think it would be wrong to change the behaviour of this crate crate because of the implementation details of a dependency, especially if returning 0 is the expected behaviour of pthread_self on Emscripten.

Perhaps thread_local can be made Emscripten-aware instead?

#[cfg(not(target_os = "emscripten"))]
fn get_thread_id() -> usize {
    thread_id::get()
}

#[cfg(target_os = "emscripten")]
fn get_thread_id() -> usize {
    1
}

[Amanieu]

This is now fixed in thread_local 0.3.1.

[BurntSushi]

Thanks @Amanieu!

[badboy]

Thanks @Amanieu! And sorry @ruuda for pushing this into the issues of your crate ;)

Happy Holidays! 🎄

[Amanieu]

I would actually feel more comfortable if the thread-id crate could guarantee that thread IDs are non-zero, or at least provide a const INVALID_THREAD_ID: usize.

I'm currently facing the need to implement this same workaround in another one of my crates, where I also store a thread ID in an AtomicUsize and use 0 as a special value to indicate no thread.

[badboy]

@Amanieu Neither pthread_self nor GetCurrentThreadId guarantees a non-zero value (pthread doesn't even guarantue that it is an arithmetic type)

[badboy]

Also, emscripten does have pthread support if enabled, in which pthread_self will return otherwise valid values

[ruuda]

@Amanieu, it sounds like you need an atomic Option<T>.

[Amanieu]

@ruuda Many platforms don't support atomic types larger than usize.