Fix assetserver_webview.go request cancelation on MacOS #3632
Conversation
WalkthroughThe recent updates enhance the handling of URL requests within the application. Key changes include the introduction of a new function to manage the stopping of URL requests, improved resource management through cancellation mechanisms, and better control flow in processing tasks related to web views. These updates collectively aim to streamline request handling, making the system more robust and responsive. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebView
participant AssetServer
User->>WebView: Initiate URL Request
WebView->>AssetServer: Create Request
AssetServer->>AssetServer: Store Cancellation Function
User->>WebView: Stop URL Request
WebView->>AssetServer: processURLRequestStop
AssetServer->>AssetServer: Cancel Request
AssetServer-->>User: Request Stopped
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- v2/internal/frontend/desktop/darwin/WailsContext.m (1 hunks)
- v2/internal/frontend/desktop/darwin/frontend.go (1 hunks)
- v2/internal/frontend/desktop/darwin/message.h (1 hunks)
- v2/pkg/assetserver/assetserver_webview.go (4 hunks)
Additional comments not posted (6)
v2/internal/frontend/desktop/darwin/message.h (1)
19-19: LGTM! The new function declaration is consistent with the existing code style.The function declaration
processURLRequestStopis consistent with the existing code style and follows the same pattern as other function declarations in the file.v2/pkg/assetserver/assetserver_webview.go (3)
24-24: LGTM! The new variable declaration is consistent with the existing code style.The variable
requestCancelFuncsMapis introduced to store cancellation functions associated with request IDs. It usessync.Mapfor thread-safe storage.
58-63: LGTM! The new function implementation is correct and follows the existing code style.The function
CancelRequestretrieves and invokes the cancellation function for a given request ID. It ensures that the cancellation function is invoked and the entry is removed from the map.
119-127: LGTM! The modifications in the method are correct and follow the existing code style.The method
processWebViewRequestInternalis modified to create a context with a cancellation function and store it inrequestCancelFuncsMap. The modifications ensure that the cancellation function is stored and invoked when needed.v2/internal/frontend/desktop/darwin/frontend.go (1)
466-475: LGTM! The new function implementation is correct and follows the existing code style.The function
processURLRequestStopcreates a new request, retrieves the request ID, and callsCancelRequest. The implementation is correct and follows the existing code style.v2/internal/frontend/desktop/darwin/WailsContext.m (1)
466-466: LGTM! EnsureprocessURLRequestStopis implemented correctly.The addition of
processURLRequestStop(self, urlSchemeTask);appears to handle necessary cleanup or processing when stopping a URL scheme task. Ensure thatprocessURLRequestStopis implemented correctly to avoid any potential issues.Verification successful
Verified: The implementation of
processURLRequestStopis correct.The function
processURLRequestStopis implemented inv2/internal/frontend/desktop/darwin/frontend.goand handles the necessary cleanup by canceling the request usingassetserver.CancelRequest(requestID).
v2/internal/frontend/desktop/darwin/frontend.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `processURLRequestStop`. # Test: Search for the implementation of `processURLRequestStop`. Expect: The function should be implemented and handle necessary cleanup or processing. rg --type objc -A 5 $'void processURLRequestStop'Length of output: 535
Script:
#!/bin/bash # Description: Locate the definition of `processURLRequestStop`. # Test: Search for the definition of `processURLRequestStop`. Expect: The function should be defined and handle necessary cleanup or processing. rg --type objc -A 10 $'void processURLRequestStop(void *self, void *urlSchemeTask)'Length of output: 84
Script:
#!/bin/bash # Description: Locate the definition of `processURLRequestStop`. # Test: Search for the definition of `processURLRequestStop`. Expect: The function should be defined and handle necessary cleanup or processing. rg --type objc -A 10 'processURLRequestStop'Length of output: 1752
Script:
#!/bin/bash # Description: Locate the definition of `processURLRequestStop`. # Test: Search for the definition of `processURLRequestStop`. Expect: The function should be defined and handle necessary cleanup or processing. rg -A 10 'processURLRequestStop'Length of output: 2611
|
Thanks for looking into this. I'm wondering whether it would be better to simply pass the URL as a string from obj-c back to Go doing something like: - (void)webView:(nonnull WKWebView *)webView stopURLSchemeTask:(nonnull id<WKURLSchemeTask>)urlSchemeTask {
const char *cURL = [urlSchemeTask.request.URL.absoluteString UTF8String]
processURLRequestStop(self, cURL);
free(cURL);
NSInputStream *stream = urlSchemeTask.request.HTTPBodyStream;
if (stream) {
NSStreamStatus status = stream.streamStatus;
if (status != NSStreamStatusClosed && status != NSStreamStatusNotOpen) {
[stream close];
}
}
}Then in Go: //export processURLRequestStop
func processURLRequestStop(_ unsafe.Pointer, requestID *C.char) {
assetserver.CancelRequest(C.GoString(requestID))
} |
|
@kopotp - Any more thoughts on this? |
1 similar comment
|
@kopotp - Any more thoughts on this? |
| //export processURLRequestStop | ||
| func processURLRequestStop(_ unsafe.Pointer, wkURLSchemeTask unsafe.Pointer) { | ||
| r := webview.NewRequest(wkURLSchemeTask) | ||
| requestID, err := r.URL() |
There was a problem hiding this comment.
The requestID shouldn't just be the r.URL() , it should also contain the r.Method().
leaanthony
added
the
awaiting feedback
Description
I am using custom Middleware with go-chi router for IPC between ui & go (need this to pass large amounts of data), and sometimes wish to cancel the request from UI before it is completed. It works for remote dev server (opening from browser via http://localhost:34115/ ) - canceling fetch request from javascript will cancel the request.Context. But this does not work from Webview - http.Request is created with empty Context.
This is my fix / proposal for MacOS
Current solution only uses request.URL() for RequestID, thinking about replacing it by extending
type Request interfacewith something like RequestID() method that will return a pointer to WKURLSchemeTask@leaanthony what do you think about this approach ?
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Improvements