diff --git a/v3/internal/assetserver/assetserver_webview.go b/v3/internal/assetserver/assetserver_webview.go index 21c01ede268..99227fb00fd 100644 --- a/v3/internal/assetserver/assetserver_webview.go +++ b/v3/internal/assetserver/assetserver_webview.go @@ -16,8 +16,22 @@ type assetServerWebView struct { // ExpectedWebViewHost is checked against the Request Host of every WebViewRequest, other hosts won't be processed. ExpectedWebViewHost string - dispatchInit sync.Once - dispatchReqC chan<- webview.Request + dispatchInit sync.Once + dispatchReqC chan<- webview.Request + + // dispatchWorkers must stay 0, which is why nothing assigns it. + // + // At 0 every request gets its own goroutine. Any positive value switches + // ServeWebViewRequest to a fixed worker pool, and a request that is + // long-lived by design - a streaming response, or anything that blocks + // waiting on the frontend - occupies its worker for its whole lifetime. + // Enough of those and the pool is starved: later requests, including the + // page's own assets, queue behind them and the app appears to hang during + // startup with no error anywhere. + // + // The field is kept because the pooled path is still useful for a + // request-heavy workload known to be short-lived, but it needs a bound on + // request lifetime before it can be turned on. dispatchWorkers int } diff --git a/v3/internal/assetserver/content_type_sniffer.go b/v3/internal/assetserver/content_type_sniffer.go index fd51a6101a6..17e12508b50 100644 --- a/v3/internal/assetserver/content_type_sniffer.go +++ b/v3/internal/assetserver/content_type_sniffer.go @@ -19,6 +19,12 @@ type contentTypeSniffer struct { status int headerCommitted bool headerWritten bool + + // err is sticky. complete may fail partway through emitting the sniffing + // prefix, and http.Flusher has no way to report that, so the failure is + // recorded here and surfaced from the next Write or complete instead of + // being lost. + err error } // Unwrap returns the wrapped [http.ResponseWriter] for use with [http.ResponseController]. @@ -31,6 +37,10 @@ func (rw *contentTypeSniffer) Header() http.Header { } func (rw *contentTypeSniffer) Write(chunk []byte) (int, error) { + if rw.err != nil { + return 0, rw.err + } + if !rw.headerCommitted { rw.WriteHeader(http.StatusOK) } @@ -108,11 +118,29 @@ func (rw *contentTypeSniffer) sniff() { // Whoever creates a contentTypeSniffer instance // is responsible for calling complete after the nested handler has returned. func (rw *contentTypeSniffer) complete() (n int, err error) { + if rw.err != nil { + return 0, rw.err + } + rw.sniff() if rw.headerWritten && len(rw.prefix) > 0 { n, err = rw.rw.Write(rw.prefix) - rw.prefix = nil + + // Drop only what actually went out. Clearing the whole prefix on a + // short or failed write would discard bytes that were never sent, and + // the caller would have no way to retry them. + if n < 0 { + n = 0 + } + if n > len(rw.prefix) { + n = len(rw.prefix) + } + rw.prefix = rw.prefix[n:] + + if err != nil { + rw.err = err + } } return @@ -135,8 +163,23 @@ func (rw *contentTypeSniffer) closeClient() { } // Flush implements the http.Flusher interface. +// +// The prefix has to be resolved first. Until 512 bytes have been seen the +// sniffer is deliberately holding the body back so it can detect a +// Content-Type, so delegating straight to the wrapped writer would flush +// nothing at all — the caller asks for a flush, gets no error, and no bytes +// reach the client. That silently breaks any response that emits less than +// 512 bytes and then waits, which is every streaming format. +// +// Completing here means the Content-Type is sniffed from a short prefix +// instead of a full one. That is the right trade: an explicit flush is the +// caller stating that what has been written so far should be sent now. func (rw *contentTypeSniffer) Flush() { + // Errors are dropped deliberately: http.Flusher cannot report one, and the + // same write error will surface from the next Write or from complete. + _, _ = rw.complete() + if f, ok := rw.rw.(http.Flusher); ok { f.Flush() } -} \ No newline at end of file +} diff --git a/v3/internal/assetserver/content_type_sniffer_test.go b/v3/internal/assetserver/content_type_sniffer_test.go new file mode 100644 index 00000000000..9214456926b --- /dev/null +++ b/v3/internal/assetserver/content_type_sniffer_test.go @@ -0,0 +1,219 @@ +package assetserver + +import ( + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// A response shorter than the 512-byte sniffing prefix is held back until the +// handler returns. Flush has to release it, or a streaming handler that writes +// a short chunk and then waits delivers nothing while reporting success. +func TestContentTypeSnifferFlushReleasesShortPrefix(t *testing.T) { + rec := httptest.NewRecorder() + rw := newContentTypeSniffer(rec) + + rw.WriteHeader(http.StatusOK) // no Content-Type set, so sniffing applies + if _, err := io.WriteString(rw, "data: first\n\n"); err != nil { + t.Fatalf("write: %v", err) + } + + if got := rec.Body.Len(); got != 0 { + t.Fatalf("prefix should still be buffered before Flush, got %d bytes", got) + } + + rw.Flush() + + if got := rec.Body.String(); got != "data: first\n\n" { + t.Errorf("Flush did not release the buffered prefix, body = %q", got) + } + if !rec.Flushed { + t.Error("Flush did not reach the wrapped writer") + } + if ct := rec.Header().Get(HeaderContentType); ct == "" { + t.Error("Content-Type was not resolved on flush") + } +} + +// After a flush the sniffer is done buffering, so later writes must pass +// straight through rather than being collected into a new prefix. +func TestContentTypeSnifferWritesPassThroughAfterFlush(t *testing.T) { + rec := httptest.NewRecorder() + rw := newContentTypeSniffer(rec) + + rw.WriteHeader(http.StatusOK) + if _, err := io.WriteString(rw, "one"); err != nil { + t.Fatalf("write: %v", err) + } + rw.Flush() + + if _, err := io.WriteString(rw, "two"); err != nil { + t.Fatalf("write after flush: %v", err) + } + if got := rec.Body.String(); got != "onetwo" { + t.Errorf("body = %q, want %q", got, "onetwo") + } +} + +// Flushing before anything is written must not panic or emit a header. +func TestContentTypeSnifferFlushBeforeWrite(t *testing.T) { + rec := httptest.NewRecorder() + rw := newContentTypeSniffer(rec) + + rw.Flush() + + if rec.Body.Len() != 0 { + t.Errorf("expected no body, got %q", rec.Body.String()) + } +} + +// An explicit Content-Type disables sniffing entirely; Flush must not disturb +// the header or duplicate the body. +func TestContentTypeSnifferFlushWithExplicitContentType(t *testing.T) { + rec := httptest.NewRecorder() + rw := newContentTypeSniffer(rec) + + rw.Header().Set(HeaderContentType, "text/event-stream") + rw.WriteHeader(http.StatusOK) + if _, err := io.WriteString(rw, "data: x\n\n"); err != nil { + t.Fatalf("write: %v", err) + } + rw.Flush() + + if got := rec.Header().Get(HeaderContentType); got != "text/event-stream" { + t.Errorf("Content-Type = %q, want text/event-stream", got) + } + if got := rec.Body.String(); got != "data: x\n\n" { + t.Errorf("body = %q", got) + } +} + +// The existing behaviour for a full prefix must be unchanged: once 512 bytes +// have accumulated the sniffer completes on its own. +func TestContentTypeSnifferFullPrefixStillCompletesWithoutFlush(t *testing.T) { + rec := httptest.NewRecorder() + rw := newContentTypeSniffer(rec) + + body := strings.Repeat("a", 600) + rw.WriteHeader(http.StatusOK) + if _, err := io.WriteString(rw, body); err != nil { + t.Fatalf("write: %v", err) + } + + if got := rec.Body.String(); got != body { + t.Errorf("body length = %d, want %d", len(got), len(body)) + } + if ct := rec.Header().Get(HeaderContentType); ct == "" { + t.Error("Content-Type was not sniffed once the prefix filled") + } +} + +// http.ResponseController must find the sniffer's Flush and report success. +func TestContentTypeSnifferSupportsResponseController(t *testing.T) { + rec := httptest.NewRecorder() + rw := newContentTypeSniffer(rec) + + rw.WriteHeader(http.StatusOK) + if _, err := io.WriteString(rw, "chunk"); err != nil { + t.Fatalf("write: %v", err) + } + + if err := http.NewResponseController(rw).Flush(); err != nil { + t.Fatalf("ResponseController.Flush: %v", err) + } + if got := rec.Body.String(); got != "chunk" { + t.Errorf("body = %q, want %q", got, "chunk") + } +} + +// failingWriter fails the nth write (1-based) and succeeds otherwise, so a +// test can target exactly the write that Flush triggers. +type failingWriter struct { + http.ResponseWriter + failOn int + writes int +} + +func (w *failingWriter) Write(b []byte) (int, error) { + w.writes++ + if w.writes == w.failOn { + return 0, errors.New("write failed") + } + return w.ResponseWriter.Write(b) +} + +func (w *failingWriter) Flush() {} + +// http.Flusher cannot report an error, so a failure while emitting the prefix +// must be remembered and surfaced from the next Write. The unwritten prefix +// must also survive rather than being silently dropped. +func TestContentTypeSnifferFlushWriteFailureIsSurfacedLater(t *testing.T) { + fw := &failingWriter{ResponseWriter: httptest.NewRecorder(), failOn: 1} + rw := newContentTypeSniffer(fw) + + rw.WriteHeader(http.StatusOK) + if _, err := io.WriteString(rw, "held back"); err != nil { + t.Fatalf("write: %v", err) + } + + rw.Flush() // triggers the prefix write, which fails + + if len(rw.prefix) == 0 { + t.Error("prefix was dropped even though the write failed") + } + + if _, err := io.WriteString(rw, "next"); err == nil { + t.Error("the flush-time failure was swallowed; later Write reported success") + } + if _, err := rw.complete(); err == nil { + t.Error("the flush-time failure was swallowed; complete reported success") + } +} + +// A short write during the prefix flush must retain only the unsent remainder, +// and the accompanying error must reach the caller afterwards. +func TestContentTypeSnifferShortWriteKeepsRemainder(t *testing.T) { + rec := httptest.NewRecorder() + sw := &shortWriter{ResponseWriter: rec, limit: 3} + rw := newContentTypeSniffer(sw) + + rw.WriteHeader(http.StatusOK) + if _, err := io.WriteString(rw, "abcdef"); err != nil { + t.Fatalf("write: %v", err) + } + rw.Flush() + + if got := rec.Body.String(); got != "abc" { + t.Errorf("wrote %q, want the accepted prefix %q", got, "abc") + } + if got := string(rw.prefix); got != "def" { + t.Errorf("remaining prefix = %q, want %q", got, "def") + } + if _, err := io.WriteString(rw, "more"); !errors.Is(err, io.ErrShortWrite) { + t.Errorf("later Write error = %v, want io.ErrShortWrite", err) + } +} + +// shortWriter accepts at most limit bytes per write. io.Writer requires a +// non-nil error whenever fewer bytes are accepted than were offered, so it +// reports io.ErrShortWrite rather than a bare short count. +type shortWriter struct { + http.ResponseWriter + limit int +} + +func (w *shortWriter) Write(b []byte) (int, error) { + if len(b) <= w.limit { + return w.ResponseWriter.Write(b) + } + n, err := w.ResponseWriter.Write(b[:w.limit]) + if err != nil { + return n, err + } + return n, io.ErrShortWrite +} + +func (w *shortWriter) Flush() {} diff --git a/v3/internal/assetserver/webview/responsewriter_darwin.go b/v3/internal/assetserver/webview/responsewriter_darwin.go index ff29a08bbb9..72eb40a1a4e 100644 --- a/v3/internal/assetserver/webview/responsewriter_darwin.go +++ b/v3/internal/assetserver/webview/responsewriter_darwin.go @@ -113,6 +113,14 @@ func (rw *responseWriter) Write(buf []byte) (int, error) { return contentLen, nil } +// Flush implements the http.Flusher interface. +// +// Write hands each chunk straight to the URL scheme task via +// didReceiveData, so there is nothing buffered on this side and a flush has +// no work to do. Declaring it anyway is what lets http.ResponseController +// report success truthfully to a streaming handler. +func (rw *responseWriter) Flush() {} + func (rw *responseWriter) WriteHeader(code int) { rw.code = code if rw.wroteHeader || rw.finished { diff --git a/v3/internal/assetserver/webview/responsewriter_ios.go b/v3/internal/assetserver/webview/responsewriter_ios.go index 7df697b6ebf..b2906ae1b82 100644 --- a/v3/internal/assetserver/webview/responsewriter_ios.go +++ b/v3/internal/assetserver/webview/responsewriter_ios.go @@ -118,6 +118,12 @@ func (rw *responseWriter) Write(buf []byte) (int, error) { return contentLen, nil } +// Flush implements the http.Flusher interface. +// +// As on macOS, Write pushes each chunk to the URL scheme task immediately, +// so there is nothing to flush. +func (rw *responseWriter) Flush() {} + func (rw *responseWriter) WriteHeader(code int) { rw.code = code if rw.wroteHeader || rw.finished { diff --git a/v3/internal/assetserver/webview/responsewriter_linux.go b/v3/internal/assetserver/webview/responsewriter_linux.go index 488bde07656..5dce0df7510 100644 --- a/v3/internal/assetserver/webview/responsewriter_linux.go +++ b/v3/internal/assetserver/webview/responsewriter_linux.go @@ -64,6 +64,13 @@ func (rw *responseWriter) Write(buf []byte) (int, error) { return rw.w.Write(buf) } +// Flush implements the http.Flusher interface. +// +// Write goes into the pipe whose read end WebKitGTK consumes as a +// GUnixInputStream, so bytes are already on their way out and there is +// nothing held back here. +func (rw *responseWriter) Flush() {} + func (rw *responseWriter) WriteHeader(code int) { rw.code = code if rw.wroteHeader || rw.finished { diff --git a/v3/internal/assetserver/webview/responsewriter_linux_gtk3.go b/v3/internal/assetserver/webview/responsewriter_linux_gtk3.go index aa4c4912d02..3af2c18b6a1 100644 --- a/v3/internal/assetserver/webview/responsewriter_linux_gtk3.go +++ b/v3/internal/assetserver/webview/responsewriter_linux_gtk3.go @@ -64,6 +64,12 @@ func (rw *responseWriter) Write(buf []byte) (int, error) { return rw.w.Write(buf) } +// Flush implements the http.Flusher interface. +// +// Write goes into the pipe WebKitGTK reads from, so nothing is buffered on +// this side. +func (rw *responseWriter) Flush() {} + func (rw *responseWriter) WriteHeader(code int) { rw.code = code if rw.wroteHeader || rw.finished { diff --git a/v3/internal/assetserver/webview/responsewriter_windows.go b/v3/internal/assetserver/webview/responsewriter_windows.go index c003f00bd83..63a07d46fd5 100644 --- a/v3/internal/assetserver/webview/responsewriter_windows.go +++ b/v3/internal/assetserver/webview/responsewriter_windows.go @@ -30,6 +30,17 @@ func (rw *responseWriter) Header() http.Header { return rw.header } +// Note: this writer deliberately does not implement http.Flusher. +// +// Unlike the other platforms, Write accumulates into an in-memory buffer that +// is only handed to WebView2 in Finish, so there is no way to push bytes to +// the client mid-response. A no-op Flush would therefore claim a capability +// that does not exist and make http.ResponseController report success while +// nothing reaches the page. Leaving it unimplemented keeps +// ResponseController.Flush returning ErrNotSupported, which is the truth. +// +// This is also why a streaming response cannot work on Windows today, +// independently of anything WebView2 does. func (rw *responseWriter) Write(buf []byte) (int, error) { if rw.finished { return 0, errResponseFinished