x/tools/internal/jsonrpc2_v2: test failures with "connection reset by peer" #46520
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
added
the
NeedsInvestigation
gopherbot
added
the
Tools
|
2021-10-04T20:57:50-db89b5a-e8a85e9/freebsd-amd64-race |
|
I don't think we're going to make progress on root causing these, but we can perhaps add a retry. |
This was referenced Jan 27, 2022
|
Change https://go.dev/cl/388134 mentions this issue: |
|
Change https://go.dev/cl/388598 mentions this issue: |
|
Change https://go.dev/cl/388594 mentions this issue: |
|
Change https://go.dev/cl/388597 mentions this issue: |
|
Change https://go.dev/cl/388600 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Mar 1, 2022
For golang/go#46520 Change-Id: Id9cdb539ae6f16e03d02f3b00b0b5ee06042a42f Reviewed-on: https://go-review.googlesource.com/c/tools/+/388594 Trust: Bryan Mills <bcmills@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Cottrell <iancottrell@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Change https://go.dev/cl/388775 mentions this issue: |
|
Change https://go.dev/cl/400054 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Apr 12, 2022
…nrpc2_v2 adapters The documentation for jsonrpc2.Replier states that “[i]f err is set then result will be ignored.” jsonrpc2_v2 documents no such affordance, and the JSON-RPC 2.0 protocol explicitly requires that the result “MUST NOT exist if there was an error invoking the method.” Although CL 388600 already avoids returning values in case of error (which may also help with escape analysis and/or allocation efficiency), it seems simplest and least confusing to make the semantic difference between the jsonrpc2.Handler and jsonrpc2_v2.Handler explicit in the code. For golang/go#46520. Change-Id: If13eb842505d42cbc51c01f5f5e699a549a3a28b Reviewed-on: https://go-review.googlesource.com/c/tools/+/400054 Run-TryBot: Bryan Mills <bcmills@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Apr 13, 2022
…n case of error The JSON-RPC 2.0 protocol requires that responses objects have either a "result" or an "error", but not both. In Go, this corresponds to a non-nil result interface value or a non-nil error. However, the generated wrappers for the LSP protocol were passing non-nil values for both in case of error, due to passing typed-nil pointers as (non-nil) interfaces (see https://go.dev/doc/faq#nil_error). This change fixes the generator to explicitly pass only one or the other, and re-runs the generator at the existing commit. For golang/go#49387 For golang/go#46520 Change-Id: I582b52820bdac15d9f947e8d6c1e9daa70c53e40 Reviewed-on: https://go-review.googlesource.com/c/tools/+/388600 Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Cottrell <iancottrell@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
Found new matching dashboard test flakes for: 2022-07-14 01:47 openbsd-386-68 tools@db8f89b3 go@558785a0 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-07-27 16:29 openbsd-386-68 tools@b3b5c13b go@ed50277f x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-07-28 19:37 freebsd-amd64-race tools@d01bb2ff go@d9242f7a x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-08-03 20:00 openbsd-386-70 tools@371fc67d go@43456202 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-08-11 16:19 openbsd-386-68 tools@37a81b68 go@133c0e90 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-08-11 18:06 openbsd-386-68 tools@c4ec74a5 go@9e4638ad x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-09-07 22:05 openbsd-amd64-70 tools@a6307518 go@2b6ff908 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-09-11 21:08 openbsd-amd64-68 tools@e71c338b go@00ece11b x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-09-19 15:32 openbsd-386-70 tools@0e011a0e go@31d06b58 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-09-19 18:18 openbsd-386-68 tools@fdf581fd go@31d06b58 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log)2022-09-19 18:40 linux-s390x-ibm tools@15782446 go@e283473e x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log) |
|
Found new dashboard test flakes for: 2022-09-28 14:34 linux-s390x-ibm tools@4dd4ddb9 go@f6d84451 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log) |
|
Found new dashboard test flakes for: 2022-09-29 22:28 openbsd-386-70 tools@d49f960b go@66165739 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log) |
|
Found new dashboard test flakes for: 2022-10-06 14:25 openbsd-amd64-68 tools@bd8c28ff go@9dfadf91 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log) |
|
Found new dashboard test flakes for: 2022-10-15 19:01 linux-s390x-ibm tools@9b5e55b1 go@947091d3 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log) |
|
Change https://go.dev/cl/443355 mentions this issue: |
|
Found new dashboard test flakes for: 2022-10-17 17:58 openbsd-386-68 tools@bc4e384f go@947091d3 x/tools/internal/jsonrpc2_v2.TestIdleTimeout (log) |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 17, 2022
…r breaks Otherwise, the Await method on the corresponding AsyncCall will never unblock, leading to a deadlock (detected by the test changes in CL 388597). For golang/go#46047 For golang/go#46520 For golang/go#49387 Change-Id: I7954f80786059772ddd7f8c98d8752d56d074919 Reviewed-on: https://go-review.googlesource.com/c/tools/+/388775 Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Cottrell <iancottrell@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 17, 2022
With the suffixes I end up a little lost in the “box” noise while I'm reading — the channel ops alone suffice to make the storage mechanism clear. (To me, the mechanism of storing a value in a 1-buffered channel is conceptually similar to storing it in a pointer, atomic.Pointer, or similar — and we don't generally name those with a suffix either.) For golang/go#46047. For golang/go#46520. For golang/go#49387. Change-Id: I7f58a9ac532f597fe49ed70606d89bd8cbe33b55 Reviewed-on: https://go-review.googlesource.com/c/tools/+/443355 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Bryan Mills <bcmills@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> 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 17, 2022
This eliminates a race between a successful Accept call and a concurrent Close call, which previously could have shut down the 'run' goroutine before Accept sent to the newConns channel, causing Accept to deadlock. In fact, it eliminates the long-running 'run' goroutine entirely (replacing it with a time.Timer), and in the process avoids leaking O(N) closed connections when only the very first one is long-lived. It also eliminates a potential double-Close bug: if the run method had called l.wrapped.Close due to an idle timeout, a subsequent call to Close would invoke l.wrapped.Close again. The io.Closer method explicitly documents doubled Close calls as undefined behavior, and many Closer implementations (especially test fakes) panic or deadlock in that case. It also eliminates a timer leak if the Listener rapidly oscillates between active and idle: previously the implementation used time.After, but it now uses an explicit time.Timer which can be stopped (and garbage-collected) when the listener becomes active. Idleness is now tracked based on the connection's Close method rather than Read: we have no guarantee in general that a caller will ever actually invoke Read (if, for example, they Close the connection as soon as it is dialed), but we can reasonably expect a caller to at least try to ensure that Close is always called. We now also verify, using a finalizer on a best-effort basis, that the Close method on each connection is called. We use the finalizer to verify the Close call — rather than to close the connection implicitly — because closing the connection in a finalizer would delay the start of the idle timer by an arbitrary and unbounded duration after the last connection is actually no longer in use. Fixes golang/go#46047. Fixes golang/go#51435. For golang/go#46520. For golang/go#49387. Change-Id: If173a3ed7a44aff14734b72c8340122e8d020f26 Reviewed-on: https://go-review.googlesource.com/c/tools/+/388597 Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Cottrell <iancottrell@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> gopls-CI: kokoro <noreply+kokoro@google.com>
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 17, 2022
For golang/go#46047 For golang/go#49387 For golang/go#46520 Change-Id: Ib72863a024d74f45c70a6cb53482cb606510f7e4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/388598 Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Cottrell <iancottrell@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> Auto-Submit: Bryan Mills <bcmills@google.com>
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 31, 2022
This change fixes the semantics of Close to actually wait for in-flight requests before closing the ReadWriteCloser. (Previously, the Close method closed the ReadWriteCloser immediately, which I suspect is what led to many of the failures observed in golang/go#49387 and golang/go#46520.) It achieves this by explicitly tracking the number of in-flight requests, including requests with pending async responses, and explicitly rejecting new Call requests (while keeping the read loop open!) once Close has begun. To make it easier for me to reason about the request lifetimes, I reduced the number of long-lived goroutines from three to just one (the Read loop), with an additional Handler goroutine that runs only while the Handler queue is non-empty. Now, it is clearer (I hope!) that the number of in-flight async requests strictly decreases after Close has begun, even though the Read goroutine continues to read requests (and, importantly, responses) and to forward Notifications to the preempter. For golang/go#49387 For golang/go#46520 Change-Id: Idf5960f848108a7ced78c5382099c8692e9b181e Reviewed-on: https://go-review.googlesource.com/c/tools/+/388134 gopls-CI: kokoro <noreply+kokoro@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com>
|
Change https://go.dev/cl/446315 mentions this issue: |
|
This should be fixed as of CL 443678 or earlier. |
Repository owner
moved this from Active
to Done
in Test Flakes
Oct 31, 2022
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Oct 31, 2022
Prior to this CL we already shut down a jsonrpc2_v2.Conn when its Reader breaks, which we expect to be the common shutdown path. However, with certain kinds of connections (notably those over stdin+stdout), it is possible for the Writer side to fail while the Reader remains working. If the Writer has failed, we have no way to return the required Response messages for incoming calls, nor to write new Request messages of our own. Since we have no way to return a response, we will now mark those incoming calls as canceled. However, even if the Writer has failed we may still be able to read the responses for any outgoing calls that are already in flight. When our in-flight calls complete, we could in theory even continue to process Notification messages from the Reader; however, those are unlikely to be useful with half the connection broken. It seems more helpful — and less surprising — to go ahead and shut down the connection completely when it becomes idle. Updates golang/go#46520. Updates golang/go#49387. Change-Id: I713f172ca7031f4211da321560fe7eae57960a48 Reviewed-on: https://go-review.googlesource.com/c/tools/+/446315 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com> Auto-Submit: Bryan Mills <bcmills@google.com> Run-TryBot: Bryan Mills <bcmills@google.com>
|
Change https://go.dev/cl/447035 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Nov 2, 2022
Prior to this CL we already shut down a jsonrpc2_v2.Conn when its Reader breaks, which we expect to be the common shutdown path. However, with certain kinds of connections (notably those over stdin+stdout), it is possible for the Writer side to fail while the Reader remains working. If the Writer has failed, we have no way to return the required Response messages for incoming calls, nor to write new Request messages of our own. Since we have no way to return a response, we will now mark those incoming calls as canceled. However, even if the Writer has failed we may still be able to read the responses for any outgoing calls that are already in flight. When our in-flight calls complete, we could in theory even continue to process Notification messages from the Reader; however, those are unlikely to be useful with half the connection broken. It seems more helpful — and less surprising — to go ahead and shut down the connection completely when it becomes idle. This is a redo of CL 446315, with additional fixes for bugs exposed on the -race builders and some extra code cleanup from the process of diagnosing those bugs. Updates golang/go#46520. Updates golang/go#49387. Change-Id: I746409a7aa2c22d5651448ed0135b5ac21a9808e Reviewed-on: https://go-review.googlesource.com/c/tools/+/447035 Auto-Submit: Bryan Mills <bcmills@google.com> Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2021-05-20T18:25:17-2275bb5/darwin-amd64-10_12
2021-04-12T17:45:26-2140cce/openbsd-amd64-68
CC @ianthehat @findleyr
The text was updated successfully, but these errors were encountered: