Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nya3jp opened this issue Nov 7, 2019 · 3 comments
Open

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

nya3jp opened this issue Nov 7, 2019 · 3 comments

Comments

@nya3jp
Copy link
Contributor

nya3jp commented Nov 7, 2019

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.

@mafredri
Copy link
Owner

mafredri commented Nov 7, 2019

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?

@nya3jp
Copy link
Contributor Author

nya3jp commented Nov 8, 2019

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-google pushed a commit to nya3jp/cdp that referenced this issue Nov 8, 2019
…ri#110)

rpcc.Conn.Close calls Target.DetachFromTarget before closing the
websocket connection, which fails if the target is already closed.
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.

This change updates the detach function to ignore errors on
Target.DetachFromTarget calls if it is because the target is already
gone.
nya3jp-google pushed a commit to nya3jp/cdp that referenced this issue Nov 8, 2019
…ri#110)

rpcc.Conn.Close calls Target.DetachFromTarget before closing the
websocket connection, which fails if the target is already closed.
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.

This change updates the detach function to ignore errors on
Target.DetachFromTarget calls if it is because the target is already
gone.

Also adds a unit test to test this behavior.

FIXME: The unit test still does not pass.
@nya3jp
Copy link
Contributor Author

nya3jp commented Nov 8, 2019

This is a naive attempt to ignore "No session with given id" error on DetachFromTarget call:
nya3jp@db30737

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 added a commit to nya3jp/tast-tests that referenced this issue Nov 8, 2019
rpcc.Conn invokes Target.DetachFromTarget before closing the connection,
which fails if the target is already closed. This error is not a real
problem, but it can confuse cautious callers who check errors of Close.

See also an upstream bug:
mafredri/cdp#110

BUG=chromium:1020484
TEST=tast run betty ui.ChromeSanity

Change-Id: I0fb1e50a683e493195874344c07f66a8ecefd40e
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/1903152
Tested-by: Shuhei Takahashi <nya@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
nya3jp added a commit to nya3jp/tast-tests that referenced this issue Nov 14, 2019
rpcc.Conn invokes Target.DetachFromTarget before closing the connection,
which fails if the target is already closed. This error is not a real
problem, but it can confuse cautious callers who check errors of Close.

See also an upstream bug:
mafredri/cdp#110

BUG=chromium:1020484
TEST=tast run betty ui.ChromeSanity

Change-Id: I0fb1e50a683e493195874344c07f66a8ecefd40e
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/1903152
Tested-by: Shuhei Takahashi <nya@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
(cherry picked from commit 38ffaee)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/1914886
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants