From 7c1490a8b118c566cf2ef2ef10fef4d174f4e83f Mon Sep 17 00:00:00 2001 From: stffabi Date: Thu, 20 Apr 2023 12:06:37 +0200 Subject: [PATCH] [assetServer] Improve release/close handling of webview requests (#2612) --- .../frontend/desktop/darwin/frontend.go | 1 - .../frontend/desktop/linux/frontend.go | 1 - v2/pkg/assetserver/assetserver_legacy.go | 12 +----- v2/pkg/assetserver/assetserver_webview.go | 12 +++--- v2/pkg/assetserver/webview/request.go | 3 +- v2/pkg/assetserver/webview/request_darwin.go | 24 +++++------ .../assetserver/webview/request_finalizer.go | 40 +++++++++++++++++++ v2/pkg/assetserver/webview/request_linux.go | 27 ++++++------- v2/pkg/assetserver/webview/responsewriter.go | 4 +- .../webview/responsewriter_darwin.go | 5 +-- .../webview/responsewriter_linux.go | 5 +-- v3/pkg/application/application.go | 4 -- 12 files changed, 78 insertions(+), 60 deletions(-) create mode 100644 v2/pkg/assetserver/webview/request_finalizer.go diff --git a/v2/internal/frontend/desktop/darwin/frontend.go b/v2/internal/frontend/desktop/darwin/frontend.go index a5517ffcb..5187be2c0 100644 --- a/v2/internal/frontend/desktop/darwin/frontend.go +++ b/v2/internal/frontend/desktop/darwin/frontend.go @@ -113,7 +113,6 @@ func (f *Frontend) startMessageProcessor() { func (f *Frontend) startRequestProcessor() { for request := range requestBuffer { f.assets.ServeWebViewRequest(request) - request.Release() } } func (f *Frontend) startCallbackProcessor() { diff --git a/v2/internal/frontend/desktop/linux/frontend.go b/v2/internal/frontend/desktop/linux/frontend.go index f8c83eac1..e02c6a928 100644 --- a/v2/internal/frontend/desktop/linux/frontend.go +++ b/v2/internal/frontend/desktop/linux/frontend.go @@ -466,7 +466,6 @@ var requestBuffer = make(chan webview.Request, 100) func (f *Frontend) startRequestProcessor() { for request := range requestBuffer { f.assets.ServeWebViewRequest(request) - request.Release() } } diff --git a/v2/pkg/assetserver/assetserver_legacy.go b/v2/pkg/assetserver/assetserver_legacy.go index 2d315aca3..4df671bc2 100644 --- a/v2/pkg/assetserver/assetserver_legacy.go +++ b/v2/pkg/assetserver/assetserver_legacy.go @@ -56,13 +56,7 @@ func (r legacyRequest) Response() webview.ResponseWriter { return &legacyRequestNoOpCloserResponseWriter{r.rw} } -func (r legacyRequest) AddRef() error { - return nil -} - -func (r legacyRequest) Release() error { - return nil -} +func (r legacyRequest) Close() error { return nil } func (r *legacyRequest) request() (*http.Request, error) { if r.req != nil { @@ -81,6 +75,4 @@ type legacyRequestNoOpCloserResponseWriter struct { http.ResponseWriter } -func (*legacyRequestNoOpCloserResponseWriter) Finish() error { - return nil -} +func (*legacyRequestNoOpCloserResponseWriter) Finish() {} diff --git a/v2/pkg/assetserver/assetserver_webview.go b/v2/pkg/assetserver/assetserver_webview.go index 3a7178c20..ae85f2513 100644 --- a/v2/pkg/assetserver/assetserver_webview.go +++ b/v2/pkg/assetserver/assetserver_webview.go @@ -22,6 +22,7 @@ type assetServerWebView struct { // ServeWebViewRequest processes the HTTP Request asynchronously by faking a golang HTTP Server. // The request will be finished with a StatusNotImplemented code if no handler has written to the response. +// The AssetServer takes ownership of the request and the caller mustn't close it or access it in any other way. func (d *AssetServer) ServeWebViewRequest(req webview.Request) { d.dispatchInit.Do(func() { workers := d.dispatchWorkers @@ -33,8 +34,11 @@ func (d *AssetServer) ServeWebViewRequest(req webview.Request) { for i := 0; i < workers; i++ { go func() { for req := range workerC { + uri, _ := req.URL() d.processWebViewRequest(req) - req.Release() + if err := req.Close(); err != nil { + d.logError("Unable to call close for request for uri '%s'", uri) + } } }() } @@ -45,12 +49,6 @@ func (d *AssetServer) ServeWebViewRequest(req webview.Request) { d.dispatchReqC = dispatchC }) - if err := req.AddRef(); err != nil { - uri, _ := req.URL() - d.logError("Unable to call AddRef for request '%s'", uri) - return - } - d.dispatchReqC <- req } diff --git a/v2/pkg/assetserver/webview/request.go b/v2/pkg/assetserver/webview/request.go index b0ce3d069..18ff29890 100644 --- a/v2/pkg/assetserver/webview/request.go +++ b/v2/pkg/assetserver/webview/request.go @@ -13,6 +13,5 @@ type Request interface { Response() ResponseWriter - AddRef() error - Release() error + Close() error } diff --git a/v2/pkg/assetserver/webview/request_darwin.go b/v2/pkg/assetserver/webview/request_darwin.go index 4f4919fab..653c19506 100644 --- a/v2/pkg/assetserver/webview/request_darwin.go +++ b/v2/pkg/assetserver/webview/request_darwin.go @@ -118,11 +118,9 @@ import ( ) // NewRequest creates as new WebViewRequest based on a pointer to an `id` -// -// Please make sure to call Release() when finished using the request. func NewRequest(wkURLSchemeTask unsafe.Pointer) Request { C.URLSchemeTaskRetain(wkURLSchemeTask) - return &request{task: wkURLSchemeTask} + return newRequestFinalizer(&request{task: wkURLSchemeTask}) } var _ Request = &request{} @@ -135,16 +133,6 @@ type request struct { rw *responseWriter } -func (r *request) AddRef() error { - C.URLSchemeTaskRetain(r.task) - return nil -} - -func (r *request) Release() error { - C.URLSchemeTaskRelease(r.task) - return nil -} - func (r *request) URL() (string, error) { return C.GoString(C.URLSchemeTaskRequestURL(r.task)), nil } @@ -205,6 +193,16 @@ func (r *request) Response() ResponseWriter { return r.rw } +func (r *request) Close() error { + var err error + if r.body != nil { + err = r.body.Close() + } + r.Response().Finish() + C.URLSchemeTaskRelease(r.task) + return err +} + var _ io.ReadCloser = &requestBodyStreamReader{} type requestBodyStreamReader struct { diff --git a/v2/pkg/assetserver/webview/request_finalizer.go b/v2/pkg/assetserver/webview/request_finalizer.go new file mode 100644 index 000000000..6a8c6a928 --- /dev/null +++ b/v2/pkg/assetserver/webview/request_finalizer.go @@ -0,0 +1,40 @@ +package webview + +import ( + "runtime" + "sync/atomic" +) + +var _ Request = &requestFinalizer{} + +type requestFinalizer struct { + Request + closed int32 +} + +// newRequestFinalizer returns a request with a runtime finalizer to make sure it will be closed from the finalizer +// if it has not been already closed. +// It also makes sure Close() of the wrapping request is only called once. +func newRequestFinalizer(r Request) Request { + rf := &requestFinalizer{Request: r} + // Make sure to async release since it might block the finalizer goroutine for a longer period + runtime.SetFinalizer(rf, func(obj *requestFinalizer) { rf.close(true) }) + return rf +} + +func (r *requestFinalizer) Close() error { + return r.close(false) +} + +func (r *requestFinalizer) close(asyncRelease bool) error { + if atomic.CompareAndSwapInt32(&r.closed, 0, 1) { + runtime.SetFinalizer(r, nil) + if asyncRelease { + go r.Request.Close() + return nil + } else { + return r.Request.Close() + } + } + return nil +} diff --git a/v2/pkg/assetserver/webview/request_linux.go b/v2/pkg/assetserver/webview/request_linux.go index ff758a065..101ee12fb 100644 --- a/v2/pkg/assetserver/webview/request_linux.go +++ b/v2/pkg/assetserver/webview/request_linux.go @@ -18,13 +18,12 @@ import ( ) // NewRequest creates as new WebViewRequest based on a pointer to an `WebKitURISchemeRequest` -// -// Please make sure to call Release() when finished using the request. func NewRequest(webKitURISchemeRequest unsafe.Pointer) Request { webkitReq := (*C.WebKitURISchemeRequest)(webKitURISchemeRequest) + C.g_object_ref(C.gpointer(webkitReq)) + req := &request{req: webkitReq} - req.AddRef() - return req + return newRequestFinalizer(req) } var _ Request = &request{} @@ -37,16 +36,6 @@ type request struct { rw *responseWriter } -func (r *request) AddRef() error { - C.g_object_ref(C.gpointer(r.req)) - return nil -} - -func (r *request) Release() error { - C.g_object_unref(C.gpointer(r.req)) - return nil -} - func (r *request) URL() (string, error) { return C.GoString(C.webkit_uri_scheme_request_get_uri(r.req)), nil } @@ -82,3 +71,13 @@ func (r *request) Response() ResponseWriter { r.rw = &responseWriter{req: r.req} return r.rw } + +func (r *request) Close() error { + var err error + if r.body != nil { + err = r.body.Close() + } + r.Response().Finish() + C.g_object_unref(C.gpointer(r.req)) + return err +} diff --git a/v2/pkg/assetserver/webview/responsewriter.go b/v2/pkg/assetserver/webview/responsewriter.go index 9e3c1952f..d67802a05 100644 --- a/v2/pkg/assetserver/webview/responsewriter.go +++ b/v2/pkg/assetserver/webview/responsewriter.go @@ -20,6 +20,6 @@ var ( type ResponseWriter interface { http.ResponseWriter - // Finish the response and flush all data. - Finish() error + // Finish the response and flush all data. A Finish after the request has already been finished has no effect. + Finish() } diff --git a/v2/pkg/assetserver/webview/responsewriter_darwin.go b/v2/pkg/assetserver/webview/responsewriter_darwin.go index 77de3c455..1c0cbee72 100644 --- a/v2/pkg/assetserver/webview/responsewriter_darwin.go +++ b/v2/pkg/assetserver/webview/responsewriter_darwin.go @@ -133,16 +133,15 @@ func (rw *responseWriter) WriteHeader(code int) { C.URLSchemeTaskDidReceiveResponse(rw.r.task, C.int(code), headers, C.int(headersLen)) } -func (rw *responseWriter) Finish() error { +func (rw *responseWriter) Finish() { if !rw.wroteHeader { rw.WriteHeader(http.StatusNotImplemented) } if rw.finished { - return nil + return } rw.finished = true C.URLSchemeTaskDidFinish(rw.r.task) - return nil } diff --git a/v2/pkg/assetserver/webview/responsewriter_linux.go b/v2/pkg/assetserver/webview/responsewriter_linux.go index 52e28aa5d..9b3f53a78 100644 --- a/v2/pkg/assetserver/webview/responsewriter_linux.go +++ b/v2/pkg/assetserver/webview/responsewriter_linux.go @@ -84,19 +84,18 @@ func (rw *responseWriter) WriteHeader(code int) { } } -func (rw *responseWriter) Finish() error { +func (rw *responseWriter) Finish() { if !rw.wroteHeader { rw.WriteHeader(http.StatusNotImplemented) } if rw.finished { - return nil + return } rw.finished = true if rw.w != nil { rw.w.Close() } - return nil } func (rw *responseWriter) finishWithError(code int, err error) { diff --git a/v3/pkg/application/application.go b/v3/pkg/application/application.go index aa2e0296c..b2667bf00 100644 --- a/v3/pkg/application/application.go +++ b/v3/pkg/application/application.go @@ -324,10 +324,6 @@ func (a *App) Run() error { for { request := <-webviewRequests a.handleWebViewRequest(request) - err := request.Release() - if err != nil { - a.error("Failed to release webview request: %s", err.Error()) - } } }() go func() {