[v3] Fix and optimise assetserver#4049
Conversation
- Minimize copying and buffering - Ensure it sniffs the full 512-bytes prefix
WalkthroughThe changes update the project's documentation and asset server implementation. The changelog now includes new entries regarding asset server fixes for response prefix size and non-404 responses. In the asset server code, several modifications improve error handling, logging, and response fallback mechanisms. Changes include updates to function signatures, switching error creation from formatted errors to simpler instantiation, and the introduction of new types and methods for managing content type sniffing and fallback responses. Minor cleanup, such as removing unnecessary imports, is also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as AssetServer
participant FRW as FallbackResponseWriter
participant Fallback as Fallback Handler
Client->>Server: Send GET request to root/index path
Server->>FRW: Process request (wrapped with fallbackResponseWriter)
FRW->>Server: Attempt to write header/status
alt Status is 404
FRW->>Fallback: Invoke fallback handler for alternative response
Fallback-->>FRW: Return fallback response
else Other status
FRW->>Client: Write normal response
end
Server->>FRW: Finalize response via complete()
FRW->>Client: Send complete response
sequenceDiagram
participant App
participant Sniffer as contentTypeSniffer
participant Writer as http.ResponseWriter
App->>Sniffer: Write(data)
alt Header not yet committed
Sniffer->>Sniffer: Buffer prefix for content sniffing
else Header committed
Sniffer->>Writer: Write data directly
end
App->>Sniffer: WriteHeader(status)
Sniffer->>Sniffer: Check for Content-Type, invoke sniff()
Sniffer->>Writer: Write header with detected Content-Type
App->>Sniffer: Call complete()
Sniffer->>Writer: Flush buffered data and complete response
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v3/internal/assetserver/options.go (1)
34-34: Adjust error message to follow Go conventionsIn Go, error messages should start with a lowercase letter unless beginning with a proper noun and should not end with punctuation. Consider modifying the error message to:
- return errors.New("AssetServer options invalid: either Handler or Middleware must be set") + return errors.New("assetserver options invalid: either Handler or Middleware must be set")v3/internal/assetserver/common.go (1)
18-19: Consider addressing the TODO comment.The Cache-Control header is essential for asset serving to control caching behavior. This TODO can likely be removed as the header's necessity is well-established.
v3/internal/assetserver/asset_fileserver.go (1)
30-37: Consider enhancing the error message structure.While the error message is informative, consider structuring it more clearly:
- msg := "no `index.html` could be found in your Assets fs.FS" + msg := "Error: index.html not found in Assets fs.FS" if embedFs, isEmbedFs := vfs.(embed.FS); isEmbedFs { rootFolder, _ := findEmbedRootPath(embedFs) - msg += fmt.Sprintf(", please make sure the embedded directory '%s' is correct and contains your assets", rootFolder) + msg += fmt.Sprintf("\nPlease verify:\n- Embedded directory '%s' is correct\n- Directory contains your assets", rootFolder) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/src/content/docs/changelog.mdx(1 hunks)v3/internal/assetserver/asset_fileserver.go(2 hunks)v3/internal/assetserver/assetserver.go(5 hunks)v3/internal/assetserver/assetserver_dev.go(0 hunks)v3/internal/assetserver/assetserver_webview.go(2 hunks)v3/internal/assetserver/common.go(2 hunks)v3/internal/assetserver/content_type_sniffer.go(1 hunks)v3/internal/assetserver/fallback_response_writer.go(1 hunks)v3/internal/assetserver/fs.go(1 hunks)v3/internal/assetserver/options.go(2 hunks)v3/internal/assetserver/webview/request_darwin.go(2 hunks)v3/internal/assetserver/webview/responsewriter_windows.go(2 hunks)
💤 Files with no reviewable changes (1)
- v3/internal/assetserver/assetserver_dev.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (17)
v3/internal/assetserver/fallback_response_writer.go (4)
22-25: Implementation ofUnwrapmethod is correctThe
Unwrapmethod correctly returns the underlyinghttp.ResponseWriter, allowing for seamless integration withhttp.ResponseController.
27-33: Proper preservation of headers inHeadermethodThe
Headermethod appropriately clones the original headers to ensure they are preserved in case a 404 response triggers the fallback mechanism.
35-43: Correct handling of writes after fallback is triggeredThe
Writemethod effectively discards further writes after the fallback has been invoked, which prevents unintended data from being sent in the response.
45-70:WriteHeadermethod properly manages status codes and fallback logicThe
WriteHeadermethod correctly identifies a 404 status code and initiates the fallback handler, ensuring that non-found resources are handled gracefully.v3/internal/assetserver/content_type_sniffer.go (1)
81-110: Content type sniffing logic is soundThe implementation of the
sniffandcompletemethods ensures that the content type is accurately determined based on the initial data prefix and that the response headers are written before the response body is sent.v3/internal/assetserver/common.go (1)
25-27: LGTM! Good improvement in type safety.Using a dedicated type for the context key instead of an empty struct literal is a better practice as it prevents potential key collisions in the context.
v3/internal/assetserver/fs.go (1)
15-15: LGTM! Good simplification of error creation.Using
errors.Newinstead offmt.Errorffor static error messages is more idiomatic and slightly more efficient.v3/internal/assetserver/webview/responsewriter_windows.go (1)
72-72: LGTM! Consider tracking the WebView2 issue.The error message indicates a workaround for a WebView2 limitation. Consider:
- Adding a link to the WebView2 issue tracker if available
- Creating a tracking issue to revisit this workaround in future WebView2 versions
✅ Verification successful
🌐 Web query:
Are there any known fixes or updates for WebView2 hanging on 304 responses?💡 Result:
The HTTP 304 "Not Modified" status code combined with WebView2 runtime issues can cause application hangs or blank pages due to caching conflicts or runtime instability. Here are verified solutions based on current technical documentation and community reports: ### Core Fixes for WebView2 Hanging on 304 Responses **1. Repair/Reinstall WebView2 Runtime** - Use Windows Control Panel → *Programs and Features* → Locate "Microsoft Edge WebView2 Runtime" → Select *Change* → Choose *Repair* [3] - If repair fails: ```bash winget install Microsoft.EdgeWebView2Runtime ``` (Requires Windows Package Manager) **2. Update Microsoft Edge Browser** WebView2 versions are tied to Edge updates: 1. Open Microsoft Edge → Go to *edge://settings/help* 2. Force an update if version < 120.x.x.x 3. Reboot system after update [6][8] **3. Server-Side Configuration Checks** If you control the backend: - Verify `Cache-Control` headers aren't set to `no-cache` or extreme durations - Ensure ETag validation works correctly with your content delivery system [1][5][7] - Test with caching disabled temporarily using: ```http Cache-Control: no-store ``` ### Advanced Troubleshooting Table | Action | Expected Outcome | Source | |--------|-------------------|--------| | Clear DNS cache via `ipconfig /flushdns` | Resolves stale DNS records affecting API calls | [4][7] | | Disable browser extensions in WebView2 | Eliminates interference with network requests | [5][10] | | Check Windows Event Viewer → *Applications* logs | Identifies WebView2 process crashes | [8] | | Test with Fiddler/Charles Proxy | Reveals malformed 304 responses from server | [1][5] | ### Recent Runtime Considerations A April 2024 WebView2 update (v120.0.2210.91+) introduced instability with conditional HTTP requests - roll back using: ```powershell winget uninstall Microsoft.EdgeWebView2Runtime winget install Microsoft.EdgeWebView2Runtime --version 119.0.2151.58 ``` [8] Where multiple systems are affected, enterprise deployments should use the Evergreen Standalone Installer with version pinning[3][6]. For persistent issues after applying these fixes, collect debug logs using: ```bash reg add HKLM\Software\Policies\Microsoft\Edge /v WebView2DebugLogging /t REG_DWORD /d 1 /f ``` Citations: - 1: https://learn.microsoft.com/en-us/answers/questions/1323561/how-do-i-fix-a-304-status-code-that-produces-a-bla - 2: https://answers.microsoft.com/en-us/windows/forum/all/getting-a-webview2-error-when-trying-to-use-new/3a8f17ef-50cb-46c6-9ecf-36d274002fe7 - 3: https://support.templafy.com/hc/en-us/articles/10510746266525-How-to-repair-Webview2-Runtime - 4: https://www.hostinger.com/tutorials/304-status-code - 5: https://hookagency.com/blog/304-status-code/ - 6: https://community.software.sil.org/t/webview2-not-updating-with-fix/8253 - 7: https://kinsta.com/knowledgebase/http-304/ - 8: https://github.com/MicrosoftEdge/WebView2Feedback/issues/4514 - 9: https://docs.citrix.com/en-us/citrix-workspace-app-for-windows/downloads/citrix_workspace_app_for_2307-1_for_windows.pdf - 10: https://10web.io/blog/http-304-not-modified-status-code/
Action Required: Add tracking for the WebView2 workaround
The current workaround is acceptable, but please add a reference (e.g., a link to the WebView2 issue tracker or an internal tracking issue) so we can revisit this once WebView2 resolves the hang on 304 responses.
- File:
v3/internal/assetserver/webview/responsewriter_windows.go(line 72)- Suggestion: Include a comment with a link or internal issue number referencing the WebView2 bug (e.g., MicrosoftEdge/WebView2Feedback#4514) for future tracking.
v3/internal/assetserver/asset_fileserver.go (2)
26-26: LGTM! Good type clarification.Using
iofs.FSinstead offs.FSmakes it clearer that this is specifically the I/O filesystem interface.
109-123: LGTM! Efficient MIME type detection.The implementation correctly uses a 512-byte buffer for MIME type detection, which is the standard size needed for accurate content type sniffing. This aligns with the PR's objective of improving the content-type sniffer.
v3/internal/assetserver/assetserver.go (3)
61-65: Improved error handling with structured logging.The deferred error logging enhances observability by providing structured context about response completion failures.
97-113: Enhanced root path handling with zero-copy fallback.The new implementation using
fallbackResponseWriterimproves efficiency by:
- Caching the accept-language header
- Providing a zero-copy fallback mechanism for root paths
- Setting appropriate content type headers
146-146: Improved error message clarity and consistency.The error messages have been updated to be more descriptive and consistent across the codebase.
Also applies to: 151-152, 163-163, 175-175
v3/internal/assetserver/assetserver_webview.go (2)
73-73: Enhanced error logging with structured format.The error logging has been updated to use a structured key-value format, improving log parsing and readability.
77-83: Improved content type handling with deferred completion.The new implementation:
- Ensures content type sniffing is always applied
- Properly handles response completion with error logging
- Maintains the fallback status code behavior
v3/internal/assetserver/webview/request_darwin.go (1)
231-233: Optimized error creation for static messages.Switched from
fmt.Errorftoerrors.Newfor static error messages, which is more efficient as it avoids unnecessary string formatting.docs/src/content/docs/changelog.mdx (1)
62-63: Well-documented changes in changelog.The changelog entries accurately reflect the improvements made to the asset server:
- Fixed response prefix size for content type sniffing
- Fixed handling of non-404 responses on root index path
a175952 to
c7cdefd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v3/internal/assetserver/fallback_response_writer.go (1)
27-33: Consider adding nil check for rw.Header()While the implementation is correct, it's good practice to add a nil check for the response writer's header.
func (fw *fallbackResponseWriter) Header() http.Header { if fw.header == nil { // Preserve original header in case we get a 404 response. + h := fw.rw.Header() + if h != nil { + fw.header = h.Clone() + } else { + fw.header = make(http.Header) + } - fw.header = fw.rw.Header().Clone() } return fw.header }v3/internal/assetserver/assetserver_test.go (2)
92-154: Consider adding timeout to prevent test hangsWhile the test implementation is solid, it's good practice to add timeouts to prevent potential test hangs.
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() - req, err := http.NewRequest(http.MethodGet, path, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, path, nil)
156-244: Consider adding negative test casesThe test covers the happy path and basic error cases well, but consider adding negative test cases:
- Invalid status codes
- Malformed paths
- Concurrent requests
Would you like me to generate additional test cases for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/src/content/docs/changelog.mdx(1 hunks)v3/internal/assetserver/assetserver.go(5 hunks)v3/internal/assetserver/assetserver_test.go(1 hunks)v3/internal/assetserver/fallback_response_writer.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/src/content/docs/changelog.mdx
- v3/internal/assetserver/assetserver.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (4)
v3/internal/assetserver/fallback_response_writer.go (3)
8-20: LGTM! Well-structured type definition with clear documentation.The
fallbackResponseWritertype is well-designed with appropriate fields for tracking state and handling fallback responses.
35-46: Consider adding error handling for implicit header writeThe Write method implicitly writes header with StatusOK, but doesn't handle potential errors from WriteHeader.
Consider whether error handling is needed for the implicit WriteHeader call. This is especially important if the fallback handler might return errors.
48-73: Verify header handling in fallback caseThe implementation looks correct, but let's verify the header handling behavior in the fallback case.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if there are any other places in the codebase where headers are handled differently for fallback cases # Search for header handling patterns in fallback scenarios rg -A 5 'Header\(\).*Clone\(\)' # Search for other response writer implementations to compare header handling ast-grep --pattern 'type $_ struct { $$$ header http.Header $$$ }'Length of output: 11678
Header handling in fallback is correctly implemented.
The investigation shows that in the fallback case (when receiving a 404), the code clones the underlying header to protect it from external modifications, consistent with similar handling in other parts of the codebase. There is no evidence of discrepancies or alternative approaches elsewhere in the repository.v3/internal/assetserver/assetserver_test.go (1)
16-91: Well-structured test cases with comprehensive coverageThe test cases cover various scenarios including:
- Simple and split content
- Short and long leading whitespace
- Custom headers and content types
- Edge cases with too long leading content
- Remove unnecessary formatting calls - Fix invalid format strings - Standardise logging calls
- Pass through non-404 responses correctly - Do not buffer original response
7fdf4b7 to
c53e0dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v3/internal/assetserver/fallback_response_writer.go (1)
35-46: Consider adding error handling for Write calls.The
Writemethod should handle potential errors from the underlyingResponseWriter.Writecall.func (fw *fallbackResponseWriter) Write(chunk []byte) (int, error) { if fw.complete { // Fallback triggered, discard further writes. return len(chunk), nil } if !fw.headerWritten { - fw.WriteHeader(http.StatusOK) + // Handle potential error from WriteHeader + fw.WriteHeader(http.StatusOK) } - return fw.rw.Write(chunk) + n, err := fw.rw.Write(chunk) + if err != nil { + return n, fmt.Errorf("write failed: %w", err) + } + return n, nil }v3/internal/assetserver/assetserver_test.go (1)
156-244: Consider parameterizing status codes and headers.The test could be more maintainable by moving the status codes and headers to constants or test parameters.
+const ( + testBody = "<html><body>Hello!</body></html>" +) + +var ( + testStatuses = []int{ + http.StatusOK, + http.StatusNotFound, + http.StatusForbidden, + } + + testHeaders = map[string][]string{ + "X-Custom": {"CustomValue"}, + } +) func TestIndexFallback(t *testing.T) { // Paths to try and whether a 404 should trigger a fallback. paths := map[string]bool{ "": true, "/": true, "/index": false, "/index.html": true, "/other": false, } - statuses := []int{ - http.StatusOK, - http.StatusNotFound, - http.StatusForbidden, - } - - header := map[string][]string{ - "X-Custom": {"CustomValue"}, - } - body := "<html><body>Hello!</body></html>"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/src/content/docs/changelog.mdx(1 hunks)v3/internal/assetserver/asset_fileserver.go(2 hunks)v3/internal/assetserver/assetserver.go(6 hunks)v3/internal/assetserver/assetserver_test.go(1 hunks)v3/internal/assetserver/assetserver_webview.go(2 hunks)v3/internal/assetserver/fallback_response_writer.go(1 hunks)v3/internal/assetserver/fs.go(1 hunks)v3/internal/assetserver/options.go(2 hunks)v3/internal/assetserver/webview/request_darwin.go(2 hunks)v3/internal/assetserver/webview/request_windows.go(2 hunks)v3/internal/assetserver/webview/responsewriter_windows.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- v3/internal/assetserver/webview/responsewriter_windows.go
- docs/src/content/docs/changelog.mdx
- v3/internal/assetserver/webview/request_darwin.go
- v3/internal/assetserver/asset_fileserver.go
- v3/internal/assetserver/fs.go
- v3/internal/assetserver/options.go
- v3/internal/assetserver/assetserver_webview.go
- v3/internal/assetserver/assetserver.go
🧰 Additional context used
📓 Learnings (1)
v3/internal/assetserver/assetserver_test.go (1)
Learnt from: fbbdev
PR: wailsapp/wails#4049
File: v3/internal/assetserver/assetserver_test.go:225-230
Timestamp: 2025-02-06T18:00:54.630Z
Learning: In assetserver tests, avoid adding assertions that depend on implementation details (like specific fallback response content) as they make tests brittle. Focus on verifying core functionality instead.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (4)
v3/internal/assetserver/fallback_response_writer.go (1)
8-20: LGTM! Well-structured type definition with clear documentation.The
fallbackResponseWritertype is well-designed with appropriate fields and clear documentation explaining its purpose.v3/internal/assetserver/webview/request_windows.go (1)
204-218: LGTM! Improved error handling with errors.Join.The implementation correctly handles error unwrapping and preserves single errors, which is a good practice for error handling.
v3/internal/assetserver/assetserver_test.go (2)
16-154: LGTM! Comprehensive test coverage for content sniffing.The test cases thoroughly cover various scenarios including edge cases with different content lengths and split writes.
225-230: Avoid implementation-dependent assertions.Based on the retrieved learnings from @fbbdev, the current check focusing on response difference is sufficient without coupling to implementation details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v3/internal/assetserver/content_type_sniffer.go (2)
24-64: Document safety assumptions for direct chunk reference.The optimization at line 42 is safe but relies on specific assumptions. Consider adding a comment explaining why storing a direct reference to
chunkis safe:
- It's only done for chunks >= 512 bytes
complete()is called immediately after, resettingprefixto nil- This optimization avoids unnecessary copying of large chunks
// Avoid copying data if a full prefix is available on first non-zero write. + // SAFETY: Storing a direct reference to chunk is safe because: + // 1. It's only done for chunks >= 512 bytes + // 2. complete() is called immediately after, which resets prefix to nil + // 3. This optimization avoids unnecessary copying of large chunks cut = len(chunk) rw.prefix = chunk chunk = nil
96-110: Enhance documentation with error handling advice.The method documentation clearly states the caller's responsibility. Consider adding a note about error handling:
// Whoever creates a contentTypeSniffer instance // is responsible for calling complete after the nested handler has returned. +// +// The caller should handle any returned error appropriately as it indicates +// a failure to write the response prefix. func (rw *contentTypeSniffer) complete() (n int, err error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/internal/assetserver/content_type_sniffer.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
v3/internal/assetserver/content_type_sniffer.go (1)
Learnt from: fbbdev
PR: wailsapp/wails#4049
File: v3/internal/assetserver/content_type_sniffer.go:38-43
Timestamp: 2025-02-06T17:58:57.120Z
Learning: In the content type sniffer of the asset server, storing a direct reference to chunks >= 512 bytes in `rw.prefix` is safe because:
1. It's only done for chunks >= 512 bytes
2. `rw.complete()` is called immediately after, which resets `rw.prefix` to nil
3. This optimization avoids unnecessary copying of large chunks in the common case
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (windows-latest, 1.23)
🔇 Additional comments (5)
v3/internal/assetserver/content_type_sniffer.go (5)
7-13: LGTM! Well-structured state management.The separation of header states into
headerCommittedandheaderWrittenprovides better control over the response lifecycle.
15-19: LGTM! Well-documented ResponseController support.The Unwrap method correctly implements the interface required for http.ResponseController.
20-22: LGTM! Consistent method receiver.The pointer receiver aligns with other methods in the struct.
66-78: LGTM! Clear header state management.The implementation correctly handles both header commitment and writing states.
80-95: LGTM! Well-structured content type detection.The method follows single responsibility principle and is well-documented.
| @@ -68,7 +69,7 @@ func (rw *responseWriter) Finish() error { | |||
| if code == http.StatusNotModified { | |||
| // WebView2 has problems when a request returns a 304 status code and the WebView2 is going to hang for other | |||
There was a problem hiding this comment.
This was committed in September 2023. I've scoured the Webview2 feedback site and can't find anything about this status code 🤔
* Fix and optimize content type sniffer - Minimize copying and buffering - Ensure it sniffs the full 512-bytes prefix * Fix assorted warnings * Cleanup error formatting - Remove unnecessary formatting calls - Fix invalid format strings - Standardise logging calls * Fix and optimize index fallback method - Pass through non-404 responses correctly - Do not buffer original response * Test content sniffing and index fallback * Update changelog * Remove obsolete check * Add safety checks in sniffer
Description
This PR fixes a couple issues in the assetserver, optimises response wrappers and provides some minor code cleanup.
Fixed issues:
/,/index,/index.html) were not being handled correctly (the status code was being forwarded, but headers and body were being discarded).Optimisations:
Cleanup:
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Ran unit tests and examples, tested various edge cases by hand, added new tests for content sniffing and index fallback.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRI have made corresponding changes to the documentationSummary by CodeRabbit
Documentation
Bug Fixes
New Features
Tests