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>

…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>

…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>

…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>

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>

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>

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>

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>

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>

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>