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

x/tools/internal/jsonrpc2_v2: eliminate heuristics for “closing errors” #56281

Closed
bcmills opened this issue Oct 17, 2022 · 3 comments
Closed
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Oct 17, 2022

x/tools/internal/jsonrpc2_v2 currently uses a set of imperfect heuristics to filter out errors that may be caused by closing a connection.

In CL 388599, @adonovan suggested that it may be simpler to report the first error that we encounter. I agree, but I think that will take a nontrivial effort to disentangle. This issue is to follow up on that cleanup (and perhaps fix any bugs that it uncovers) once my main stack of jsonrpc2_v2 fixes lands.

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository. labels Oct 17, 2022
@bcmills bcmills added this to the Backlog milestone Oct 17, 2022
@bcmills bcmills self-assigned this Oct 18, 2022
@gopherbot
Copy link

Change https://go.dev/cl/443675 mentions this issue: internal/jsonrpc2_v2: eliminate isClosingErr

@gopherbot
Copy link

Change https://go.dev/cl/443678 mentions this issue: internal/jsonrpc2_v2: rename Serve to NewServer and eliminate its error return

@gopherbot
Copy link

Change https://go.dev/cl/443677 mentions this issue: internal/jsonrpc2_v2: eliminate error return from Bind

gopherbot pushed a commit to golang/tools that referenced this issue Oct 31, 2022
Also make (*Connection).Close safe to call from Bind.

A jsonrpc2_v2.Server has no good way to report an error from Bind.

If the Server saves the error to return from its own Wait method, that
might not ever actually happen: Wait waits for in-flight connections
to complete, but if some existing connection stays up then Wait will
not return.

If the Server goes ahead with establishing the connection and installs
its own Handler, that Handler needs to decide whether to serve the
error from Bind or something more opaque, and at that point Bind may
as well return a handler that makes that choice more precisely.

If the Server merely logs the error and closes the Connection, then
the Bind method itself may as well do that directly too.

It seems to me that the only winning move is not to play. Only Bind is
in a position to decide how to handle its errors appropriately, so it
should not return them to the Server.

Updates golang/go#56281.

Change-Id: I07dc43ddf31253ce23da21a92d2b6c0f8d4b3afe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443677
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Oct 31, 2022
…or return

Serve had a misleading name and signature: it did not actually block
on serving the connection, and never returned a non-nil error.

Updates golang/go#56281.

Change-Id: Ia6df0ba20066811b0551df3b3267dff2fffd7881
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443678
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Oct 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

2 participants