Repository navigation
Conversation
The Linux response writers stream the body to WebKit through a pipe created with Pipe2(p, 0). Without O_CLOEXEC, any child process forked while a response is in flight (os/exec, a helper the app spawns) inherits the write end. The handler returns and closes its copy, but WebKit only sees EOF once every copy is closed, so the body (and the fetch() behind a binding call) stalls until that child exits, even though headers arrived on time. Create the pipe with O_CLOEXEC on both ends. The read end is only consumed in-process by the GUnixInputStream, so nothing relies on inheriting it. Applies to both the GTK4 and GTK3 writers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughBoth Linux response pipe implementations now set close-on-exec. A Linux test checks that both pipe descriptors have the flag set. ChangesLinux response pipe
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change prevents child processes started through exec from keeping response pipes open after the writer finishes. The inspected GTK response flow remains intact, so no merge-blocking concern remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each pipe with care, Comment |
|
Automated v3 GA-readiness test result for head
This change is Linux-only, so the code it touches (the asset pipe O_CLOEXEC path) was not exercised by this run. The run only confirms that the PR does not break macOS or Windows builds and tests, and those results match the master baseline at |
This branch has not been deployed
2 participants
Description
On Linux, the asset server's response writers (
responsewriter_linux.gofor GTK4,responsewriter_linux_gtk3.gofor GTK3) stream the response body to WebKit through a pipe created withsyscall.Pipe2(p, 0). Because the pipe has noO_CLOEXEC, any child process forked while a response is in flight inherits the write end. This includesos/exec, a helper the app restarts, and so on. The handler returns and closes its own copy, but WebKit only sees EOF on the body once every copy of the write end is closed. So the body, and thefetch()behind a binding call, stalls until that unrelated child exits, even though the response headers arrived on time.In a real app this showed up as a binding call (
GetMessages) whose result never reached JS. The Go method had returned in ~60 ms, but the promise resolved 9–17 s later, exactly when the app killed a helper process it had (re)started at the same moment. Instrumenting/procconfirmed it: during every stall, the helper held the write end of the response pipe, and the body completed the instant the helper died.This PR creates the pipe with
O_CLOEXECon both ends. The read end is only consumed in-process by theGUnixInputStream, so nothing relies on it being inherited. The original comment ("we especially don't want to have the FD_CLOEXEC") gave no reason, and I could not find one. It is replaced by an explanation of why the flag is required.Type of change
How Has This Been Tested?
Unit test:
TestPipeIsCloseOnExecchecksFD_CLOEXECon both ends ofpipe(). It fails onmasterand passes with this change, on both the default (GTK4) and-tags gtk3builds.go test -race ./internal/assetserver/...passes on both builds.End-to-end repro (minimal app, no frontend framework): a bound method returns an N KiB string. While it returns, a goroutine starts a few short-lived children, simulating an app that spawns helpers concurrently:
The page calls
Call.ByName("main.Repro.Big", kb)with an idle gap between calls and measures the time until the promise resolves. 6 calls per size (1, 16, 64, 256, 512, 1024 KiB), 36 calls per run:master-tags gtk3)Without the concurrent
exec, both builds show 0 stalls. That is expected, since the bug needs a fork while the pipe is open.Test Configuration
Checklist:
Summary by CodeRabbit