cdp icon indicating copy to clipboard operation
cdp copied to clipboard

rpcc.Conn.Close returns an error if the target is already closed

Open nya3jp opened this issue 6 years ago • 3 comments

rpcc.Conn.Close returns an error if the target is already closed, e.g. by invoking cdp.Client.Target.CloseTarget. This is a bit inconvenient because we have to ignore errors from rpcc.Conn.Close when we close the target in advance, which means that we might miss other interesting errors.

nya3jp avatar Nov 07 '19 07:11 nya3jp

I see what you mean, I hadn't considered closing a target before the rpcc.Conn. I think we can detect this and close the conn without error. This means that rpcc.ErrConnClosing would be returned (as when calling rpcc.Conn.Close twice), would that be OK?

mafredri avatar Nov 07 '19 09:11 mafredri

Hi Mathias, thanks for quick response!

It's great if rpcc.Conn.Close returns no error as long as the connection is successfully closed. In particular, I want rpcc.Conn.Close to ignore DetachFromTarget failure if it is about the target already gone.

Since the websocket connection is bi-directional, it should be closed the both party (client and server). And there's no strict requirement on which party to initiate closing the connection. If the server (browser) gracefully closes the connection first, the client (cdp) has to close the connection as well, and I don't expect it to result in an error (of course, if there is in-flight method calls, they should fail with errors).

What do you think?

nya3jp avatar Nov 08 '19 02:11 nya3jp

This is a naive attempt to ignore "No session with given id" error on DetachFromTarget call: https://github.com/nya3jp/cdp/commit/db3073764ad69d92741d71573ff38f8c25877ba5

However it does not pass my new unit test with the following error:

=== RUN   TestManager_CloseTarget
--- FAIL: TestManager_CloseTarget (0.19s)
    session_test.go:222: rpcc: the connection is closing

IIUC it is because rpc.Conn.close is automatically called when the websocket connection is closed.

nya3jp avatar Nov 08 '19 02:11 nya3jp