Prevent panicking from goroutine if a read error is encountered.

[pallavagarwal07]

Currently on MacOS, closing a window causes a panic inside a goroutine, which can't be recovered from. This is due to c.Close() being called more than once in the readResponses() method that encounters the error.

This change tries to propagate that error and breaks out of the for loop instead.

[pallavagarwal07]

Actually I see there is already a more elaborate pull request by jezek for this purpose. #44

I'm wondering if this, as a tiny provable pull request might be better since you mention you are uncomfortable merging the other pull request.

[jezek]

Hello. I didn't only wrote elaborate fix, I also wrote even more elaborate tests using dummy X server. There are different test scenarios, the scenario for this bug is called TestConnOnNonBlockingDummyXServer/unexpected_conn_close.

Runntnig the test scenario on unpatched xgb results in:

# go test -run TestConnOnNonBlockingDummyXServer/unexpected_conn_close
XGB: xgb.go:403: A read error is unrecoverable: EOF
XGB: xgb.go:403: A read error is unrecoverable: EOF
panic: close of closed channel

goroutine 25 [running]:
github.com/jezek/xgb.(*Conn).Close(...)
	/home/jezek/Programy/go/src/github.com/jezek/xgb/xgb.go:140
github.com/jezek/xgb.(*Conn).readResponses(0xc0000b82c0)
	/home/jezek/Programy/go/src/github.com/jezek/xgb/xgb.go:405 +0x25a
created by github.com/jezek/xgb.postNewConn
	/home/jezek/Programy/go/src/github.com/jezek/xgb/xgb.go:133 +0x206
exit status 2
FAIL	github.com/jezek/xgb	0.008s

and we can see the panic. The test didn't run until end, the panic exited the program prematurely. (Note: the panic was impossible to catch and recover from in tests, cause it happened in a goroutine).

If you run the same test on your patch, you will get:

# go test -run TestConnOnNonBlockingDummyXServer/unexpected_conn_close
XGB: xgb.go:419: A read error is unrecoverable: EOF
XGB: xgb.go:383: A write error is unrecoverable: server closed
--- FAIL: TestConnOnNonBlockingDummyXServer (0.01s)
    --- FAIL: TestConnOnNonBlockingDummyXServer/unexpected_conn_close (0.01s)
        xgb_test.go:201: WaitForEvent() = (<nil>, EOF), want (nil, nil)
        xgb_test.go:210: (*Conn).Close() panic recover: close of closed channel
        testingTools.go:106: after server close, before testcase exit: github.com/jezek/xgb.(*Conn).generateXIds(0xc0000b82c0) is leaking
        testingTools.go:106: after server close, before testcase exit: github.com/jezek/xgb.(*Conn).generateSeqIds(0xc0000b82c0) is leaking
        testingTools.go:106: after server close, before testcase exit: github.com/jezek/xgb.(*Conn).sendRequests(0xc0000b82c0) is leaking
FAIL
exit status 1
FAIL	github.com/jezek/xgb	0.016s

As you can see, there is no more panic, the test finished, but failed. The first two fail occurences

        xgb_test.go:201: WaitForEvent() = (<nil>, EOF), want (nil, nil)
        xgb_test.go:210: (*Conn).Close() panic recover: close of closed channel
        

can be ignored, cause the tests expect <nil, nil> results and you introduced new error type. Since there was no <nil, nil> the test assumes the connection was not closed and closes it causing a panic which was recovered from.

The other test fails are leaking goroutines. So if you don't open/close multiple times, the leaks don't matter and your patch should be fine. I think, the PR #39 tried to fix the leaks and the panic too, but there was som flaw (I don't currently remember what flaw it was) so I decided to write my panic and leaks solving solution, which resulted in #44.

[pallavagarwal07]

Hey jezek,

Thanks for testing this out! We actually switched over to using your fork when we realized that the fix already existed (I hadn't really tested my patch extensively (or at all)). Cheers!