runtime: windows callback on non-Go thread fails

[alexbrainman]

This test

diff --git a/src/pkg/runtime/syscall_windows_test.go
b/src/pkg/runtime/syscall_windows_test.go
--- a/src/pkg/runtime/syscall_windows_test.go
+++ b/src/pkg/runtime/syscall_windows_test.go
@@ -240,5 +240,35 @@
 }
 
 func TestCallbackInAnotherThread(t *testing.T) {
-   // TODO: test a function which calls back in another thread: QueueUserAPC() or
CreateThread()
+   d := GetDLL(t, "kernel32.dll")
+
+   f := func(p uintptr) uintptr {
+       return p
+   }
+   r, _, err := d.Proc("CreateThread").Call(0, 0, syscall.NewCallback(f), 123,
0, 0)
+   if r == 0 {
+       t.Fatalf("CreateThread failed: %v", err)
+   }
+   h := syscall.Handle(r)
+   defer syscall.CloseHandle(h)
+
+   switch s, err := syscall.WaitForSingleObject(h, 100); s {
+   case syscall.WAIT_OBJECT_0:
+       break
+   case syscall.WAIT_TIMEOUT:
+       t.Fatal("timeout waiting for thread to exit")
+   case syscall.WAIT_FAILED:
+       t.Fatalf("WaitForSingleObject failed: %v", err)
+   default:
+       t.Fatalf("WaitForSingleObject returns unexpected value %v", s)
+   }
+
+   var ec uint32
+   r, _, err = d.Proc("GetExitCodeThread").Call(uintptr(h),
uintptr(unsafe.Pointer(&ec)))
+   if r == 0 {
+       t.Fatalf("GetExitCodeThread failed: %v", err)
+   }
+   if ec != 123 {
+       t.Fatalf("expected 123, but got %d", ec)
+   }
 }

fails with

C:\>u:\test -test.v -test.run=Another
=== RUN TestCallbackInAnotherThread
--- FAIL: TestCallbackInAnotherThread (0.11 seconds)
        syscall_windows_test.go:259: timeout waiting for thread to exit
FAIL

hg id is 39c724dd7f25

Alex

PS: for after Go 1.2

[alexbrainman]

Comment 1:

The issue is raised here
https://groups.google.com/d/topic/golang-nuts/h91eDx27vrQ/discussion
Alex

[gopherbot]

Comment 2 by vanillabluesky007:

Looks great. 
Probably the same issue that i triggered. The only difference in my case is that my dll
uses C-Runtime therefore creates threads with _beginthreadex rather than CreateThread.
(different thread management in both)
See:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682453(v=vs.85).aspx 
 
(P.s nice trick calling kernel32.dll directly and apologies for being late on opening
the issue!)
/Sven

[alexbrainman]

Comment 3:

sven,
Attaching your test case here for posterity. I could not use your dll, it uses
msvcr110d.dll that I don't have on my amd64 pc. And I couldn't build it on my 386 pc,
because _beginthread expects __cdecl, not __stdcall function (it, probably, didn't
matter for you, if you're on amd64).
Anyway, I think my test is good enough for our purpose. Don't you agree?
I investigated where my test hangs. It is in lockextra, like you said. And it is because
no free m. cgo does not have that problem, because it sets runtime·needextram to 1 at
the start. I don't know why it is limited to cgo only. But if I move it into runtime,
then my test PASSes. I suspect your program should work too.
Here is my diff:
diff --git a/src/pkg/runtime/cgo/iscgo.c b/src/pkg/runtime/cgo/iscgo.c
--- a/src/pkg/runtime/cgo/iscgo.c
+++ b/src/pkg/runtime/cgo/iscgo.c
@@ -12,4 +12,3 @@
 #include "../runtime.h"
 
 bool runtime·iscgo = 1;
-uint32 runtime·needextram = 1;  // create an extra M on first cgo call
diff --git a/src/pkg/runtime/crash_test.go b/src/pkg/runtime/crash_test.go
--- a/src/pkg/runtime/crash_test.go
+++ b/src/pkg/runtime/crash_test.go
@@ -105,7 +105,7 @@
    testDeadlock(t, lockedDeadlockSource)
 }
 
-func TestLockedDeadlock2(t *testing.T) {
+func testLockedDeadlock2(t *testing.T) {
    testDeadlock(t, lockedDeadlockSource2)
 }
 
diff --git a/src/pkg/runtime/proc.c b/src/pkg/runtime/proc.c
--- a/src/pkg/runtime/proc.c
+++ b/src/pkg/runtime/proc.c
@@ -64,7 +64,7 @@
 
 Sched  runtime·sched;
 int32  runtime·gomaxprocs;
-uint32 runtime·needextram;
+uint32 runtime·needextram = 1;
 bool   runtime·iscgo;
 M  runtime·m0;
 G  runtime·g0;     // idle goroutine for m0
diff --git a/src/pkg/runtime/syscall_windows_test.go
b/src/pkg/runtime/syscall_windows_test.go
--- a/src/pkg/runtime/syscall_windows_test.go
+++ b/src/pkg/runtime/syscall_windows_test.go
@@ -240,5 +240,35 @@
 }
 
 func TestCallbackInAnotherThread(t *testing.T) {
-   // TODO: test a function which calls back in another thread: QueueUserAPC() or
CreateThread()
+   d := GetDLL(t, "kernel32.dll")
+
+   f := func(p uintptr) uintptr {
+       return p
+   }
+   r, _, err := d.Proc("CreateThread").Call(0, 0, syscall.NewCallback(f), 123, 0, 0)
+   if r == 0 {
+       t.Fatalf("CreateThread failed: %v", err)
+   }
+   h := syscall.Handle(r)
+   defer syscall.CloseHandle(h)
+
+   switch s, err := syscall.WaitForSingleObject(h, 100); s {
+   case syscall.WAIT_OBJECT_0:
+       break
+   case syscall.WAIT_TIMEOUT:
+       t.Fatal("timeout waiting for thread to exit")
+   case syscall.WAIT_FAILED:
+       t.Fatalf("WaitForSingleObject failed: %v", err)
+   default:
+       t.Fatalf("WaitForSingleObject returns unexpected value %v", s)
+   }
+
+   var ec uint32
+   r, _, err = d.Proc("GetExitCodeThread").Call(uintptr(h), uintptr(unsafe.Pointer(&ec)))
+   if r == 0 {
+       t.Fatalf("GetExitCodeThread failed: %v", err)
+   }
+   if ec != 123 {
+       t.Fatalf("expected 123, but got %d", ec)
+   }
 }
I had to disable one test. I don't know why, so we have to resolve that.
We cannot make changes now, not until after Go 1.2 is released. Perhaps this change will
get accepted. I hope you can use this in the meantime to fix your problem locally.
Alex

Attachments:

1. multithread.go (1138 bytes)
2. multithreaded.zip (6031 bytes)

[rsc]

Comment 4:

Labels changed: added go1.3maybe.

[rsc]

Comment 5:

Labels changed: added release-none, removed go1.3maybe.

[rsc]

Comment 6:

Labels changed: added repo-main.

[chowey]

Comment 7:

This one bit me today.
import "C" does solve the issue. What is the status on merging this into the main trunk
of Go?

[alexbrainman]

Comment 8:

cameron.howey,
My patch at #3 is not good. Like I said there: "... I had to disable one test. I don't
know why, so we have to resolve that. ...". It appears the test fails with "deadlock"
message, so we need to rearrange scheduler somewhat before we can apply this patch.
I didn't push for it, because I don't have anyone complaining about it. Perhaps, if it
is important to you, you could tell us what your difficulty is. Maybe that will convince
someone to put some actual effort into it. Thank you.
Alex

[chowey]

Comment 9:

I wanted to use Windows COM (specifically the MMDevice and EndpointVolume APIs) from a
Go project. If this sounds like a bad idea, it is. So I wrote a simple DLL in C++ for
the COM stuff and exported the relevant functions. I have the DLL start another thread
and run the COM stuff from there, to avoid worrying about how COM was going to interact
with Go's thread-hopping, etc.
I use IAudioEndpointVolume::RegisterEndpointNotificationCallback to get notifications
when the system volume changes. I wanted to use a Go callback here.
So the issue is that the DLL runs a separate thread but needs to use a Go callback in
that thread. I was not otherwise using Cgo, instead using syscall to call my DLL.
Like I said, using import "C" solved the issue. It works like a champ. But I spent some
hours trying to figure it out beforehand (the Go executable just crashed with no
message) until I found this on the issue tracker.
Possibly you don't hear about this issue much because most Go programmers don't use
callbacks in a separate thread in a DLL on Windows. Furthermore, people who ARE doing
this might already use import "C", which would hide the problem.

[alexbrainman]

Comment 10:

cameron.howey,
> I wanted to use Windows COM (specifically the MMDevice and EndpointVolume APIs) from a
Go project. If this sounds like a bad idea, it is. So I wrote a simple DLL in C++ for
the COM stuff and exported the relevant functions. ...
I would like to see what you are trying to do. You can provide a C program, if you like.
I am interested to see, if I can convert it into pure Go. Just for fun. When I have
time. Thank you.
Alex

[chowey]

Comment 11:

I posted a gist: https://gist.github.com/chowey/6705c5c6b07399e6a73a

[alexbrainman]

Comment 12:

Thank you for the gist.
Alex