Fix for multi threaded callback crash on Windows.

[unbornchikken]

See this one: #199

[TooTallNate]

So rather than using the uv functions on Windows, we're just using the native Windows API for checking the thread? This seems like a more fundamental issue in libuv to me. Have you reported it upstream yet?

[unbornchikken]

I've took a look into this, and found that it's not a bug but a lazy implementation. On Linux libuv threading is just a wrapper on top of pthreads, so uv_thread_self just calls pthread_self, and because of this every thread has a valid identity which is returned from there even the thread wasn't created by libuv. On Windows uv_thread_self returns a pointer to a custom structure from thread local storage that created by libuv but of course this value only exists for threads created by libuv, so this will returns null for other threads. I think they assumed that if the mentioned value doesn't exists in the thread local storage, it will reports an error, so uv_get_key will abort. According to the MSDN documentation it won't be always the case:

TlsGetValue was implemented with speed as the primary goal. The function performs minimal parameter validation and error checking. In particular, it succeeds if dwTlsIndex is in the range 0 through (TLS_MINIMUM_AVAILABLE– 1). It is up to the programmer to ensure that the index is valid and that the thread calls TlsSetValue before calling TlsGetValue.

Which means it might reports and error if the value is not present or might not, depends on the situation. This explains that why I called it some unobvious circumstances in my issue report.

So since libuv's mentioned function is good for libuv thread identification, it is not a bug at their side. Simply we should not rely uv_thread_self to identitfy threads on Windows. (But it is interesting that node's main thread is not created by libuv API, but I can imagine some limitations those can explain this.)

[TooTallNate]

Considering that libuv is a platform normalization library, I would expect the semantics of uv_thread_self() to be identical on Unix vs. Windows. This still sounds like a libuv bug IMO.

[unbornchikken]

I think if a platform normalization library cannot identify constructs those are created by-passed them, it is forgivable.

[TooTallNate]

Summoning @piscisaureus to help give me an opinion here…

[piscisaureus]

Indeed, libuv doesn't currently support calling uv_thread_self() from a non-uv thread. That's something that could be fixed though.

But in all honesty, I don't really see much added value in using libuv for this purpose. You'd probably better just be pragmatic and write a thin wrapper yourself:

uintptr_t get_thread_id() {
#ifdef _WIN32
  return (uintptr_t) GetCurrentThreadId();
#else
  return (uintptr_t) pthread_self();
#endif
}

... and use that to compare threads. Formally, pthread IDs have to be compared with pthread_equal(), and pthread_t might be a struct, but in practice the above always works.

• Bert

[unbornchikken]

That's the solution that my PR uses, thanks.

[piscisaureus]

@unbornchikken

That's the solution that my PR uses.

Well, kind of. This doesn't look too logical to me though:

+  uv_thread_t self_thread = (uv_thread_t) uv_thread_self();
+#ifdef WIN32
+  if (g_threadID == GetCurrentThreadId()) {
+#else
   if (uv_thread_equal(&self_thread, &g_mainthread)) {
+#endif    

It looks like you indended to do the right thing though :)
Anyhow, I'm not a reviewer in this project. @TooTallNate, take it away!

[unbornchikken]

Yeah, I misplaced the first line that should be unix only. But since libuv returns pthread on unix, under the cover its the same. I'll fix that later, and everyone should be happy, it fixes the crash definitely anyway. ;)

[TooTallNate]

Ok fix up that last line and I'll merge

On Wednesday, April 8, 2015, Gábor Mező notifications@github.com wrote:

Yeah, I misplaced the first line that should be unix only. But since libuv
returns pthread on unix, under the cover its the same. I'll fix that later,
and everyone should be happy, it fixes the crash definitely anyway. ;)

—
Reply to this email directly or view it on GitHub
#200 (comment).

[unbornchikken]

It's done.

[TooTallNate]

@unbornchikken Ok, I've merged in fc7aacc.

Regarding this issue, is it something possible to create a test case for? That would be ideal, but I understand that the edge-case nature of this might make that difficult/impossible.

I also added you as a committer to this repo, so we can get stuff merged faster. I only ask that we continue to add stuff in pull requests just to get >= 2 pairs of eyes on the changes. Great work lately though! Much appreciated.

[unbornchikken]

It's a geat honor to me, thanks! I try to get some time to do a test case for covering this. Since it's a Windows only issue, doing native Windows threading will trigger this for sure.