Remove dependencies on the eventtest and testenv packages from
jsonrpc2_v2 and mcp packages, in preparation for forking.
Change-Id: Id0efb0c3187629f6fb1822cc853ec52acea41b0d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/682696
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL contains an minimal prototype of an MCP SDK, using the
"new" jsonrpc2_v2 package (similar to x/exp/jsonrpc2).
Much is still yet to do, but this initial version addressed the
following aspects:
- Support for newline delimited JSONRPC2 framing, and message logging.
- A generated protocol package, containing type definitions to be used
in transport.
- A minimal jsonschema package to be used for both reading the MCP spec,
and for serving Tool input schemas.
- A transport abstraction, and two transport implementations: stdio, and
local (for testing).
- Client and Server types, to be configured with features and then
connected using their Connect methods, which return (respectively) a
ServerConnection and ClientConnection object.
- A minimal binding API for tools.
A catalog of things not yet done is in doc.go, but we should review this
CL eagerly, as it contains the fundamental building blocks of an
eventual SDK.
It is the intention that this package is more-or-less standalone, as it
may eventually be carved out into a separate repository. As such, its
only dependency within x/tools is jsonrpc2_v2, and the only change
required to that package is to expose a callback when the connection is
closed.
Change-Id: Ib66018961014f85a8884d56ab721d5642c98443c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/667036
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Produced with a patched version of modernize containing only
the efaceany pass:
$ go run ./gopls/internal/analysis/modernize/cmd/modernize/main.go -test -fix ./... ./gopls/...
This is very safe and forms the bulk of the modernize diff,
isolating the other changes for ease of review.
Change-Id: Iec9352bac5a639a5c03368427531c7842c6e9ad0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/650759
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
testenv.HasExec should not assume that "js" and "ios" are the only
platforms that can't exec. Instead of hard-coding a list of GOOS,
hard-code only the ones known to work and probe the remainder by
trying to re-exec the test binary.
Similarly, in testenv_notunix, negate the list of unix platforms
instead of hard-coding the non-unix ones.
Add missing calls to testenv functions were needed.
For golang/go#58141.
Updates golang/go#59718.
Change-Id: I0114e0bfb6d091e84b325d7f9bb0896da22482be
Reviewed-on: https://go-review.googlesource.com/c/tools/+/486315
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Commit-Queue: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The only explanation I can think of for the failure in
https://go.dev/issue/49387#issuecomment-1303979877 is that maybe the
idle timeout started as soon as conn1 was closed, without waiting for
conn2 to be closed. That might be possible if the connection returned
by Dial was still in the server's accept queue, but never actually
accepted.
To eliminate that possibility, we can send an RPC on that connection
and wait for a response, as we already do with conn1. Since the conn1
RPC succeeded, we know that the connection is non-idle, conn2 should
be accepted, and the request on conn2 should succeed unconditionally.
Fixesgolang/go#49387 (hopefully for real this time).
Change-Id: Ie3e74f91d322223d82c000fdf1f3a0ed08afd20d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448096
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
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>
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>
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>
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.
Fixesgolang/go#46047.
Fixesgolang/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>
Update the new binder tests to run both with a standalone server, and
with a forwarding chain.
Make a few superficial improvements along the way as well.
Change-Id: Icd197698093a3f6149ab58171806b2388ed75b7f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/321134
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
There was already handling in isClosingErr for errors from Stdin/Stdout
and TCP connections. Add handling for io.Pipe closing errors.
Change-Id: I8d171ab49a3fffe0fca9f0482f6b92d61b1fae1f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/320849
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
