From 705ba3e00b1613f70dadb876f834c40a36710501 Mon Sep 17 00:00:00 2001 From: Codinget Date: Sun, 30 Aug 2026 23:32:56 +0000 Subject: [PATCH] fix(tsconnect): address bridge review findings Co-Authored-By: gpt-5.6-sol --- cmd/tsconnect/wasm/bridge_test.go | 99 +++++++++++++++++++++++++++++-- cmd/tsconnect/wasm/drive.go | 46 ++++++++------ cmd/tsconnect/wasm/taildrop.go | 28 ++++++--- 3 files changed, 143 insertions(+), 30 deletions(-) diff --git a/cmd/tsconnect/wasm/bridge_test.go b/cmd/tsconnect/wasm/bridge_test.go index 104f959c3..8a5ed122d 100644 --- a/cmd/tsconnect/wasm/bridge_test.go +++ b/cmd/tsconnect/wasm/bridge_test.go @@ -42,10 +42,19 @@ func TestDriveHandlerThrowReturns500(t *testing.T) { } func TestDriveCancellationDisablesCallbacks(t *testing.T) { - started := make(chan struct{}) + type handlerArgs struct { + req js.Value + res js.Value + } + started := make(chan handlerArgs, 1) + deferred := evalJS(t, `(() => { + let resolve + const promise = new Promise(r => { resolve = r }) + return {promise, resolve} + })()`) handler := js.FuncOf(func(this js.Value, args []js.Value) any { - close(started) - return nil + started <- handlerArgs{req: args[0], res: args[1]} + return deferred.Get("promise") }) defer handler.Release() fs := new(jsFileSystemForRemote) @@ -59,8 +68,9 @@ func TestDriveCancellationDisablesCallbacks(t *testing.T) { close(done) }() + var jsArgs handlerArgs select { - case <-started: + case jsArgs = <-started: case <-time.After(time.Second): t.Fatal("drive handler did not start") } @@ -71,6 +81,21 @@ func TestDriveCancellationDisablesCallbacks(t *testing.T) { t.Fatal("drive handler did not stop after cancellation") } + readResult := jsArgs.req.Call("readBodyChunk") + if readResult.Type() != js.TypeObject { + t.Fatalf("readBodyChunk after cancellation returned %s, want Promise", readResult.Type()) + } + if result := awaitFulfilledPromise(t, readResult); !result.IsNull() { + t.Fatalf("readBodyChunk after cancellation = %v, want null", result) + } + lateChunk := js.Global().Get("Uint8Array").New(1) + js.CopyBytesToJS(lateChunk, []byte("x")) + jsArgs.res.Call("write", lateChunk) + if recorder.Body.Len() != 0 { + t.Fatalf("late JS write changed response body to %q", recorder.Body.String()) + } + deferred.Call("resolve") + writeStarted := make(chan struct{}) allowWrite := make(chan struct{}) response := &driveResponse{ @@ -106,6 +131,23 @@ func TestDriveCancellationDisablesCallbacks(t *testing.T) { } } +func TestDriveHandlerFulfillmentCompletesRequest(t *testing.T) { + handler := evalJS(t, `(function() { return Promise.resolve() })`) + fs := new(jsFileSystemForRemote) + fs.setHandler(handler) + recorder := httptest.NewRecorder() + done := make(chan struct{}) + go func() { + fs.ServeHTTPWithPerms(nil, recorder, httptest.NewRequest("GET", "/", nil)) + close(done) + }() + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("fulfilled drive handler did not complete request") + } +} + func TestJSStreamReaderErrorsAreSticky(t *testing.T) { for _, test := range []struct { name string @@ -142,6 +184,23 @@ func TestJSStreamReaderCloseErrors(t *testing.T) { } } +func TestJSStreamReaderCloseTimesOut(t *testing.T) { + r := &jsStreamReader{ + reader: evalJS(t, `({cancel() { return new Promise(() => {}) }})`), + testCancelTimeout: 10 * time.Millisecond, + } + started := time.Now() + if err := r.Close(); err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("Close error = %v, want timeout", err) + } + if elapsed := time.Since(started); elapsed >= 250*time.Millisecond { + t.Fatalf("Close took %v, want less than 250ms", elapsed) + } + if _, err := r.Read(make([]byte, 1)); err == nil || !strings.Contains(err.Error(), "timed out") { + t.Fatalf("Read error after Close timeout = %v, want sticky timeout", err) + } +} + func TestJSStreamReaderIgnoresDuplicateSettlement(t *testing.T) { reader := evalJS(t, `({ read() { @@ -233,3 +292,35 @@ func awaitPromise(t *testing.T, promise js.Value) js.Value { return js.Undefined() } } + +func awaitFulfilledPromise(t *testing.T, promise js.Value) js.Value { + t.Helper() + type result struct { + value js.Value + rejected bool + } + ch := make(chan result, 1) + resolve := js.FuncOf(func(this js.Value, args []js.Value) any { + ch <- result{value: args[0]} + return nil + }) + reject := js.FuncOf(func(this js.Value, args []js.Value) any { + ch <- result{value: args[0], rejected: true} + return nil + }) + defer resolve.Release() + defer reject.Release() + if _, err := callJSMethod(promise, "then", resolve, reject); err != nil { + t.Fatal(err) + } + select { + case result := <-ch: + if result.rejected { + t.Fatalf("promise rejected with %v; want fulfillment", result.value) + } + return result.value + case <-time.After(time.Second): + t.Fatal("promise did not settle") + return js.Undefined() + } +} diff --git a/cmd/tsconnect/wasm/drive.go b/cmd/tsconnect/wasm/drive.go index ba88a7de5..15eace69f 100644 --- a/cmd/tsconnect/wasm/drive.go +++ b/cmd/tsconnect/wasm/drive.go @@ -138,7 +138,7 @@ func (fs *jsFileSystemForRemote) ServeHTTPWithPerms( readBodyChunk := js.FuncOf(func(_ js.Value, _ []js.Value) any { if !response.isLive() { return makePromise(func() (any, error) { - return nil, errors.New("drive request is closed") + return js.Null(), nil }) } return makePromise(func() (any, error) { @@ -176,26 +176,35 @@ func (fs *jsFileSystemForRemote) ServeHTTPWithPerms( } return nil }) - rejected := js.FuncOf(func(_ js.Value, args []js.Value) any { + var fulfilled, rejected js.Func + var handlerSettled sync.Once + settleHandler := func(err error) { + handlerSettled.Do(func() { + select { + case response.doneCh <- err: + default: + } + readBodyChunk.Release() + writeHead.Release() + write.Release() + end.Release() + fulfilled.Release() + rejected.Release() + }) + } + fulfilled = js.FuncOf(func(_ js.Value, _ []js.Value) any { + settleHandler(nil) + return nil + }) + rejected = js.FuncOf(func(_ js.Value, args []js.Value) any { err := errors.New("JavaScript drive handler rejected") if len(args) > 0 && args[0].Type() == js.TypeString { err = fmt.Errorf("JavaScript drive handler rejected: %s", args[0].String()) } - select { - case response.doneCh <- err: - default: - } + settleHandler(err) return nil }) - defer func() { - readBodyChunk.Release() - writeHead.Release() - write.Release() - end.Release() - rejected.Release() - }() - jsReq := map[string]any{ "method": r.Method, "path": r.URL.Path, @@ -214,14 +223,13 @@ func (fs *jsFileSystemForRemote) ServeHTTPWithPerms( if hasThen, err := hasJSFunctionProperty(result, "then"); err != nil { handlerErr = err } else if hasThen { - _, handlerErr = callJSMethod(result, "then", js.Undefined(), rejected) + _, handlerErr = callJSMethod(result, "then", fulfilled, rejected) + } else { + settleHandler(nil) } } if handlerErr != nil { - select { - case response.doneCh <- handlerErr: - default: - } + settleHandler(handlerErr) } var resultErr error diff --git a/cmd/tsconnect/wasm/taildrop.go b/cmd/tsconnect/wasm/taildrop.go index 8e46dc0aa..439537fdd 100644 --- a/cmd/tsconnect/wasm/taildrop.go +++ b/cmd/tsconnect/wasm/taildrop.go @@ -246,13 +246,16 @@ func wireTaildropFileOps(lb *ipnlocal.LocalBackend, jsObj js.Value) { // ReadableStreamDefaultReader. Each Read call awaits one reader.read() Promise, // using the channel+FuncOf pattern so Go blocks until JS delivers the chunk. type jsStreamReader struct { - mu sync.Mutex - reader js.Value - buf []byte - done bool - err error + mu sync.Mutex + reader js.Value + buf []byte + done bool + err error + testCancelTimeout time.Duration } +const jsStreamCancelTimeout = time.Second + func (r *jsStreamReader) Read(p []byte) (int, error) { r.mu.Lock() if r.err != nil { @@ -386,8 +389,19 @@ func (r *jsStreamReader) Close() error { if _, err := callJSMethod(promise, "then", resolveFn, rejectFn); err != nil { return r.setError(err) } - if err := <-resultCh; err != nil { - return r.setError(err) + timeout := jsStreamCancelTimeout + if r.testCancelTimeout > 0 { + timeout = r.testCancelTimeout + } + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case err := <-resultCh: + if err != nil { + return r.setError(err) + } + case <-timer.C: + return r.setError(errors.New("JavaScript stream cancel timed out")) } r.mu.Lock() r.done = true