WS error handling
vkorn opened this issue · 8 comments
Greetings. First of all thank you for a great library.
I have kinda a mix of a bug/feature request.
What version of go-chrome are you using (tag, hash, etc.)?
v1.0.0-rc5
What behavior do you expect? What are you trying to accomplish?
While using remote debug connection if you close chrome library crashes with panic during WS read attempt
error 2018-09-19T21:31:18.958-07:00 0000: An unknown error occurred
⇢ socketID=1
⇢ socket.socketer.go:305 github.com/mkenney/go-chrome/tot/socket.(*Socket).listen
error 2018-09-19T21:31:18.958-07:00 nil response from socket
⇢ socketID=1
⇢ socket.socketer.go:314 github.com/mkenney/go-chrome/tot/socket.(*Socket).listen
error 2018-09-19T21:31:18.959-07:00 Unknown response from web socket
⇢ data="{\\\"error\\\":null,\\\"id\\\":0,\\\"method\\\":\\\"\\\",\\\"params\\\":null,\\\"result\\\":null}" method="" responseID=0 socketID=1
⇢ socket.socketer.go:338 github.com/mkenney/go-chrome/tot/socket.(*Socket).listen
panic: repeated read on failed websocket connection
goroutine 25 [running]:
github.com/gorilla/websocket.(*Conn).NextReader(0xc0000f4c60, 0x10, 0x10, 0x13f6ba0, 0xc0001715c8, 0x100da98)
/Users/vkorn/Documents/Work/private/go-home/go-home/pkg/mod/github.com/gorilla/websocket@v1.4.0/conn.go:967 +0x356
github.com/gorilla/websocket.(*Conn).ReadJSON(0xc0000f4c60, 0x13ae0e0, 0xc0002e40c0, 0x0, 0x0)
/Users/vkorn/Documents/Work/private/go-home/go-home/pkg/mod/github.com/gorilla/websocket@v1.4.0/json.go:50 +0x2f
github.com/mkenney/go-chrome/tot/socket.(*ChromeWebSocket).ReadJSON(0xc000098b80, 0x13ae0e0, 0xc0002e40b0, 0x0, 0xc000156400)
/Users/vkorn/Documents/Work/private/go-home/go-home/pkg/mod/github.com/mkenney/go-chrome@v1.0.0-rc5/tot/socket/socket.web_socketer.go:81 +0x7e
github.com/mkenney/go-chrome/tot/socket.(*Socket).ReadJSON(0xc000154000, 0x139a320, 0xc0000aa2d8, 0x1, 0xc000171b20)
/Users/vkorn/Documents/Work/private/go-home/go-home/pkg/mod/github.com/mkenney/go-chrome@v1.0.0-rc5/tot/socket/socket.conner.go:99 +0xc1
github.com/mkenney/go-chrome/tot/socket.(*Socket).listen(0xc000154000, 0x0, 0x0)
/Users/vkorn/Documents/Work/private/go-home/go-home/pkg/mod/github.com/mkenney/go-chrome@v1.0.0-rc5/tot/socket/socket.socketer.go:301 +0x131
created by github.com/mkenney/go-chrome/tot/socket.(*Socket).Listen
/Users/vkorn/Documents/Work/private/go-home/go-home/pkg/mod/github.com/mkenney/go-chrome@v1.0.0-rc5/tot/socket/socket.socketer.go:287 +0x70
What behavior are you experiencing instead?
Socket re-connect should be handled correctly. In addition it would be great to receive some kind of callbacks so we can re-open a tab if it was a chrome crash.
How can this behavior be reproduced?
- Open a new tab
- time.Sleep for a few seconds
- Try to get a screenshot/anything else
While process waits for # 2, terminate Chrome.
Hi @vkorn,
That's interesting, thank you for reporting. I agree, that's a little finicky, but at some point the retry should still give up (though not with a panic).
Do you (or anyone else) have any thoughts on what the preferred behavior would be when your connection to a remote chrome instance is tenuous or experiencing network instability?
It probably depends on a way the library is used. For example, I have a long-running process which opens a new Chrome tab using remote API, and periodically takes screenshot of a page. In this scenario it would be great to have some kind of callback so I can try to re-open this page. Chrome itself is dockerized so if socket is broken due to Chrome crash, I don't care and just need to wait till docker restarts.
Number of re-connect retries might be just a param.
Ok, that adds color, that's not a use-case I was thinking of. A callback signal of some kind makes sense.
I'll take a look and see what I can come up with.
Hi @vkorn,
I took a look and decided to add an error channel you can use to inspect error activity in the socket. I'm not sure if this is the ideal approach so feedback is appreciated.
Here's an example script, I tested it by stopping chrome while it was waiting for events. It intercepts the panic and feeds the error into the error channel with an error code you can key off of (codes.SocketPanic).
Please let me know your thoughts.
Just tested, everything works as expected, I guess, the issue can be closed after merge.
Thank you again.
@vkorn that's done, let me know if you have any problems. Thanks!
https://github.com/mkenney/go-chrome/releases/tag/v1.0.0-rc6