syscall: fix data races in LazyDLL/LazyProc

syscall: fix data races in LazyDLL/LazyProc

[brainman, 2012-11-07 03:50:03]

LGTM It is "ugly", as you said, but most sensible in the circumstances. Thank you. Add "Fixes issue 4343" to the CL description. Alex https://codereview.appspot.com/6817086/diff/2001/src/pkg/syscall/dll_windows.go File src/pkg/syscall/dll_windows.go (right): https://codereview.appspot.com/6817086/diff/2001/src/pkg/syscall/dll_windows.... src/pkg/syscall/dll_windows.go:179: atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&d.dll)), unsafe.Pointer(dll)) I do not think you need to change that line. d.mu.Lock up above should protect code executing here. https://codereview.appspot.com/6817086/diff/2001/src/pkg/syscall/dll_windows.... src/pkg/syscall/dll_windows.go:234: atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&p.proc)), unsafe.Pointer(proc)) Same. Do not change that line.

[brainman, 2012-11-07 05:04:47]

I just applied https://codereview.appspot.com/6810080/ to test this change and it still fails in a similar way (while running test in debug/pe)? On 2012/11/07 03:50:03, brainman wrote: > LGTM > Perhaps, I should say NOT LGTM. > > https://codereview.appspot.com/6817086/diff/2001/src/pkg/syscall/dll_windows.... > src/pkg/syscall/dll_windows.go:179: > atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&d.dll)), > unsafe.Pointer(dll)) > I do not think you need to change that line. d.mu.Lock up above should protect > code executing here. > I take this back too. I realized now that we do need atomic.StorePointer here. Alex

[dvyukov2, 2012-11-07 05:27:25]

On 2012/11/07 05:04:47, brainman wrote: > I just applied https://codereview.appspot.com/6810080/ to test this change and > it still fails in a similar way (while running test in debug/pe)? How exactly does it fail? Do you mean that it prints the race report. What do you mean by debug/pe? > > On 2012/11/07 03:50:03, brainman wrote: > > LGTM > > > > Perhaps, I should say NOT LGTM. > > > > > > https://codereview.appspot.com/6817086/diff/2001/src/pkg/syscall/dll_windows.... > > src/pkg/syscall/dll_windows.go:179: > > atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&d.dll)), > > unsafe.Pointer(dll)) > > I do not think you need to change that line. d.mu.Lock up above should protect > > code executing here. > > > > I take this back too. I realized now that we do need atomic.StorePointer here. > > Alex

[brainman, 2012-11-07 05:31:54]

On 2012/11/07 05:27:25, dvyukov2 wrote: > > How exactly does it fail? ... c:\MinGW64\go\src\pkg\debug\pe>go test -v -race === RUN TestOpen --- PASS: TestOpen (0.03 seconds) === RUN TestOpenFailure ================== WARNING: DATA RACE Read by goroutine 4: syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:252 +0x6e syscall.CloseHandle() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:302 +0x5b os.(*file).close() c:/mingw64/go/src/pkg/os/file_windows.go:147 +0x230 os.(*File).Close() c:/mingw64/go/src/pkg/os/file_windows.go:136 +0x46 debug/pe.Open() c:/mingw64/go/src/pkg/debug/pe/file.go:104 +0xe0 debug/pe.TestOpenFailure() c:/mingw64/go/src/pkg/debug/pe/file_test.go:123 +0x4e testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 Previous write by goroutine 3: syscall.(*DLL).FindProc() c:/mingw64/go/src/pkg/syscall/dll_windows.go:85 +0x35a syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:230 +0x158 syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:242 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:251 +0x35 syscall.CloseHandle() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:302 +0x5b os.(*file).close() c:/mingw64/go/src/pkg/os/file_windows.go:147 +0x230 runfinq() c:/mingw64/go/src/pkg/runtime/mgc0.c:1170 +0x13d Goroutine 4 (running) created at: testing.RunTests() c:/mingw64/go/src/pkg/testing/testing.go:377 +0xafb testing.Main() c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 main.main() C:/Users/brainman/AppData/Local/Temp/go-build070889751/debug/pe/_test/_tes tmain.go:45 +0xdd runtime.main() c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 Goroutine 3 (running) created at: runtime.gc() c:/mingw64/go/src/pkg/runtime/mgc0.c:961 +0x0 os.(*File).writeConsole() c:/mingw64/go/src/pkg/os/file_windows.go:272 +0x772 os.(*File).write() c:/mingw64/go/src/pkg/os/file_windows.go:291 +0x110 os.(*File).Write() c:/mingw64/go/src/pkg/os/file.go:139 +0xd2 fmt.Fprintf() c:/mingw64/go/src/pkg/fmt/print.go:214 +0xe1 fmt.Printf() c:/mingw64/go/src/pkg/fmt/print.go:222 +0xcf testing.RunTests() c:/mingw64/go/src/pkg/testing/testing.go:375 +0xa90 testing.Main() c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 main.main() C:/Users/brainman/AppData/Local/Temp/go-build070889751/debug/pe/_test/_tes tmain.go:45 +0xdd runtime.main() c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 ================== --- PASS: TestOpenFailure (0.01 seconds) PASS Found 1 data race(s) exit status 66 FAIL debug/pe 0.166s c:\MinGW64\go\src\pkg\debug\pe> > ... Do you mean that it prints the race report. Yes, it prints above message even after I apply this CL. > What do you mean by debug/pe? I run "go test -v -race" command in debug/pe. Alex

[dvyukov2, 2012-11-07 09:12:09]

On 2012/11/07 05:31:54, brainman wrote: > On 2012/11/07 05:27:25, dvyukov2 wrote: > > > > How exactly does it fail? ... > > c:\MinGW64\go\src\pkg\debug\pe>go test -v -race > === RUN TestOpen > --- PASS: TestOpen (0.03 seconds) > === RUN TestOpenFailure > ================== > WARNING: DATA RACE > Read by goroutine 4: > syscall.(*LazyProc).Addr() > c:/mingw64/go/src/pkg/syscall/dll_windows.go:252 +0x6e > syscall.CloseHandle() > c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:302 +0x5b > os.(*file).close() > c:/mingw64/go/src/pkg/os/file_windows.go:147 +0x230 > os.(*File).Close() > c:/mingw64/go/src/pkg/os/file_windows.go:136 +0x46 > debug/pe.Open() > c:/mingw64/go/src/pkg/debug/pe/file.go:104 +0xe0 > debug/pe.TestOpenFailure() > c:/mingw64/go/src/pkg/debug/pe/file_test.go:123 +0x4e > testing.tRunner() > c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 > > Previous write by goroutine 3: > syscall.(*DLL).FindProc() > c:/mingw64/go/src/pkg/syscall/dll_windows.go:85 +0x35a > syscall.(*LazyProc).Find() > c:/mingw64/go/src/pkg/syscall/dll_windows.go:230 +0x158 > syscall.(*LazyProc).mustFind() > c:/mingw64/go/src/pkg/syscall/dll_windows.go:242 +0x35 > syscall.(*LazyProc).Addr() > c:/mingw64/go/src/pkg/syscall/dll_windows.go:251 +0x35 > syscall.CloseHandle() > c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:302 +0x5b > os.(*file).close() > c:/mingw64/go/src/pkg/os/file_windows.go:147 +0x230 > runfinq() > c:/mingw64/go/src/pkg/runtime/mgc0.c:1170 +0x13d > > Goroutine 4 (running) created at: > testing.RunTests() > c:/mingw64/go/src/pkg/testing/testing.go:377 +0xafb > testing.Main() > c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 > main.main() > C:/Users/brainman/AppData/Local/Temp/go-build070889751/debug/pe/_test/_tes > tmain.go:45 +0xdd > runtime.main() > c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 > > Goroutine 3 (running) created at: > runtime.gc() > c:/mingw64/go/src/pkg/runtime/mgc0.c:961 +0x0 > os.(*File).writeConsole() > c:/mingw64/go/src/pkg/os/file_windows.go:272 +0x772 > os.(*File).write() > c:/mingw64/go/src/pkg/os/file_windows.go:291 +0x110 > os.(*File).Write() > c:/mingw64/go/src/pkg/os/file.go:139 +0xd2 > fmt.Fprintf() > c:/mingw64/go/src/pkg/fmt/print.go:214 +0xe1 > fmt.Printf() > c:/mingw64/go/src/pkg/fmt/print.go:222 +0xcf > testing.RunTests() > c:/mingw64/go/src/pkg/testing/testing.go:375 +0xa90 > testing.Main() > c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 > main.main() > C:/Users/brainman/AppData/Local/Temp/go-build070889751/debug/pe/_test/_tes > tmain.go:45 +0xdd > runtime.main() > c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 > > ================== > --- PASS: TestOpenFailure (0.01 seconds) > PASS > Found 1 data race(s) > exit status 66 > FAIL debug/pe 0.166s > > c:\MinGW64\go\src\pkg\debug\pe> > > > ... Do you mean that it prints the race report. > > Yes, it prints above message even after I apply this CL. Sorry, that's actually another issue with finalizers (you can see that one of the stacks is from finalizer goroutine). But it's great that the race detector works for you on Windows. Thanks for testing! OK, let's tackle them one by one. First, I will submit first Windows version. Then, fix finalizers. Then, get back to this bug.

[brainman, 2012-11-08 03:45:14]

Testing hg id 7fac8ec2f901 now that you've submitted your race facility, I can reliably break compress/lzw and net tests: c:\MinGW64\go\src>go test -race -short compress/lzw ================== WARNING: DATA RACE Write by goroutine 3: syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:232 +0x194 syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:240 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:249 +0x35 syscall.ReadFile() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:263 +0xba syscall.Read() c:/mingw64/go/src/pkg/syscall/syscall_windows.go:263 +0xb4 os.(*File).read() c:/mingw64/go/src/pkg/os/file_windows.go:216 +0xfb os.(*File).Read() c:/mingw64/go/src/pkg/os/file.go:95 +0xd2 bytes.(*Buffer).ReadFrom() c:/mingw64/go/src/pkg/bytes/buffer.go:166 +0x4ba io/ioutil.readAll() c:/mingw64/go/src/pkg/io/ioutil/ioutil.go:32 +0x1c6 io/ioutil.ReadAll() c:/mingw64/go/src/pkg/io/ioutil/ioutil.go:41 +0x4d compress/lzw.testFile() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:66 +0x6a8 compress/lzw.TestWriter() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:93 +0x216 testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 Previous read by goroutine 4: syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:220 +0x4d syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:240 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:249 +0x35 syscall.ReadFile() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:263 +0xba syscall.Read() c:/mingw64/go/src/pkg/syscall/syscall_windows.go:263 +0xb4 os.(*File).read() c:/mingw64/go/src/pkg/os/file_windows.go:216 +0xfb os.(*File).Read() c:/mingw64/go/src/pkg/os/file.go:95 +0xd2 compress/lzw.func·001() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:47 +0x1f4 Goroutine 3 (running) created at: testing.RunTests() c:/mingw64/go/src/pkg/testing/testing.go:377 +0xb0c testing.Main() c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 main.main() C:/Users/brainman/AppData/Local/Temp/go-build238292983/compress/lzw/_test/_testmain.go:59 +0xdd runtime.main() c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 Goroutine 4 (running) created at: compress/lzw.testFile() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:61 +0x5b6 compress/lzw.TestWriter() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:93 +0x216 testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 ================== ================== WARNING: DATA RACE Write by goroutine 3: syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:232 +0x194 syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:240 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:249 +0x35 syscall.CloseHandle() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:304 +0x5b os.(*file).close() c:/mingw64/go/src/pkg/os/file_windows.go:147 +0x230 os.(*File).Close() c:/mingw64/go/src/pkg/os/file_windows.go:136 +0x46 compress/lzw.testFile() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:26 +0x2c0 compress/lzw.TestWriter() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:93 +0x216 testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 Previous read by goroutine 4: syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:220 +0x4d syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:240 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:249 +0x35 syscall.CloseHandle() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:304 +0x5b os.(*file).close() c:/mingw64/go/src/pkg/os/file_windows.go:147 +0x230 os.(*File).Close() c:/mingw64/go/src/pkg/os/file_windows.go:136 +0x46 compress/lzw.func·001() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:54 +0x4b0 Goroutine 3 (running) created at: testing.RunTests() c:/mingw64/go/src/pkg/testing/testing.go:377 +0xb0c testing.Main() c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 main.main() C:/Users/brainman/AppData/Local/Temp/go-build238292983/compress/lzw/_test/_testmain.go:59 +0xdd runtime.main() c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 Goroutine 4 (running) created at: compress/lzw.testFile() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:61 +0x5b6 compress/lzw.TestWriter() c:/mingw64/go/src/pkg/compress/lzw/writer_test.go:93 +0x216 testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 ================== PASS Found 2 data race(s) FAIL compress/lzw 2.592s c:\MinGW64\go\src>go test -race -short net ================== WARNING: DATA RACE Write by goroutine 45: syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:232 +0x194 syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:240 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:249 +0x35 syscall.WSARecv() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:1412 +0x66 net.(*readOp).Submit() c:/mingw64/go/src/pkg/net/fd_windows.go:404 +0xdc net.(*ioSrv).ExecIO() c:/mingw64/go/src/pkg/net/fd_windows.go:173 +0xbb net.(*netFD).Read() c:/mingw64/go/src/pkg/net/fd_windows.go:420 +0x1d9 net.(*conn).Read() c:/mingw64/go/src/pkg/net/net_posix.go:30 +0x115 net.TestShutdown() c:/mingw64/go/src/pkg/net/net_test.go:53 +0x58c testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 Previous read by goroutine 46: syscall.(*LazyProc).Find() c:/mingw64/go/src/pkg/syscall/dll_windows.go:220 +0x4d syscall.(*LazyProc).mustFind() c:/mingw64/go/src/pkg/syscall/dll_windows.go:240 +0x35 syscall.(*LazyProc).Addr() c:/mingw64/go/src/pkg/syscall/dll_windows.go:249 +0x35 syscall.WSARecv() c:/mingw64/go/src/pkg/syscall/zsyscall_windows_amd64.go:1412 +0x66 net.(*readOp).Submit() c:/mingw64/go/src/pkg/net/fd_windows.go:404 +0xdc net.(*ioSrv).ExecIO() c:/mingw64/go/src/pkg/net/fd_windows.go:173 +0xbb net.(*netFD).Read() c:/mingw64/go/src/pkg/net/fd_windows.go:420 +0x1d9 net.(*conn).Read() c:/mingw64/go/src/pkg/net/net_posix.go:30 +0x115 net.func·021() c:/mingw64/go/src/pkg/net/net_test.go:34 +0x1df Goroutine 45 (running) created at: testing.RunTests() c:/mingw64/go/src/pkg/testing/testing.go:377 +0xb0c testing.Main() c:/mingw64/go/src/pkg/testing/testing.go:313 +0xd0 main.main() C:/Users/brainman/AppData/Local/Temp/go-build123861631/net/_test/_testmain.go:189 +0xdd runtime.main() c:/mingw64/go/src/pkg/runtime/proc.c:248 +0x94 Goroutine 46 (running) created at: net.TestShutdown() c:/mingw64/go/src/pkg/net/net_test.go:40 +0x24e testing.tRunner() c:/mingw64/go/src/pkg/testing/testing.go:301 +0x92 ================== PASS Found 1 data race(s) FAIL net 1.423s but after applying CL6817086, these failures disapper. So, I would say LGTM. Please, add "Fixes issue 4343" to CL description. Also, you, probably, want to redo this CL, because you've sent it as d...@gmail.com, so it does not find your in CONTRIBUTORS file. Alex