Refactor CDP #422
Refactor CDP #422
Conversation
Adding HTTP & websocket awareness to the TCP server. HTTP server handles `GET /json/version` and websocket upgrade requests. Conceptually, websocket handling is the same code as before, but receiving data will parse the websocket frames and writing data will wrap it in a websocket frame. The previous `Ctx` was split into a `Server` and a `Client`. This was largely done to make it easy to write unit tests, since the `Client` is a generic, all its dependencies (i.e. the server) can be mocked out. This also makes it a bit nicer to know if there is or isn't a client (via the server's client optional). Added a MemoryPool for the Send object (I thought that was a nice touch!) Removed MacOS hack on accept/conn completion usage. Known issues: - When framing an outgoing message, the entire message has to be duped. This is no worse than how it was before, but it should be possible to eliminate this in the future. Probably not part of this PR. - Websocket parsing will reject continuation frames. I don't know of a single client that will send a fragmented message (websocket has its own message fragmentation), but we should probably still support this just in case. - I don't think the receive, timeout and close completions can safely be re-used like we're doing. I believe they need to be associated with a specific client socket. - A new connection creates a new browser session. I think this is right (??), but for the very first, we're throwing out a perfectly usable session. I'm thinking this might be a change to how Browser/Sessions work. - zig build test won't compile. This branch reproduces the issue with none of these changes: https://github.com/karlseguin/browser/tree/broken_test_build (or, as a diff to main): lightpanda-io/browser@main...karlseguin:broken_test_build
Move more logic into the reader. Avoid copying partial messages in cases where we know that the buffer is large enough. This is mostly groundwork for trying to add support for continuation frames.
CDP is now an struct which contains its own state a browser and a session. When a client connection is made and successfully upgrades, the client creates the CDP instance. There is now a cleaner separation betwen Server, Client and CDP. Removed a number of allocations, especially when writing results/events from CDP to the client. Improved input message parsing. Tried to remove some usage of undefined.
|
Hi @karlseguin, first of all thanks for this PR, it's impressive. It seems that there is a regression (leak) on memory usage. When I launch 1000 runs of the puppeteer demo on my MBA M2, looking Activity Monitor I have:
|
In the page cdp, I was creating a copy of the page, thus copying the arena, which causes issues. I had removed There's still a leak on Mac from the cancel callback not being called and these allocations not being freed, but this predates the PR. |
Fixes issue where CDP closes the client, but client still registers a recv operation.
|
Great, that fixes the leak, thanks. I'm reviewing the code itself but it's a pretty big PR so it takes some time :) |
| pub fn init(self: *Browser, alloc: std.mem.Allocator, loop: *Loop, vm: jsruntime.VM) !void { | ||
| // We want to ensure the caller initialised a VM, but the browser | ||
| // doesn't use it directly... | ||
| _ = vm; |
There was a problem hiding this comment.
I think it's useful to request a vm as parameter here to force the caller to create one before calling.
There was a problem hiding this comment.
Before, there was one global browser, assigned directly to the server.
Now there's 1 browser per client. Once this is merged, I was planning on making CDP multi-browser (I think when Target.createBrowserContext, we should create a new browser - else if a client creates multiple contexts, they won't get the isolation that they should (i.e. cookies/storage)).
Having this check means storing the VM instance in the Server, in the Client, and eventually in CDP. If this is important, how about a global boolean in zig-js-runtime which is set to true on vm.init and which Env.init checks every time?
There was a problem hiding this comment.
AFAIK vm.Init https://github.com/lightpanda-io/zig-js-runtime/blob/main/src/engines/v8/v8.zig#L73-L78 calls multiple v8's functions to initiate v8 before any isolate is created (https://github.com/lightpanda-io/zig-v8-fork/blob/fork/src/v8.zig#L78-L91, https://github.com/lightpanda-io/zig-v8-fork/blob/fork/src/v8.zig#L117-L122, https://github.com/lightpanda-io/zig-v8-fork/blob/fork/src/v8.zig#L124-L129)
Now there's 1 browser per client.
Got it 👍
If this is important, how about a global boolean in zig-js-runtime which is set to true on vm.init and which Env.init checks every time?
I guess it's me, I found weird to have to init a vm and never pass it somewhere 😅
But using a boolean is ok too. The problem w/o this check is the error if you don't init the vm isn't very explicit:
$ zig build run
#
# Fatal error in ../../../../v8/src/api/api.cc, line 388
# Check failed: i::GetProcessWideSandbox()->is_initialized().
#
#
#
#FailureMessage Object: 0x7ffe9094fbe0run
└─ run lightpanda failure
error: the following command terminated unexpectedly:
/home/pierre/wrk/browser/.zig-cache/o/b20fc1513f2dcc377631adee7a63ca0f/lightpanda
Build Summary: 2/4 steps succeeded; 1 failed (disable with --summary none)
run transitive failure
└─ run lightpanda failure
error: the following build command failed with exit code 1:
/home/pierre/wrk/browser/.zig-cache/o/7a9935469ab9b5af96ef2ed0fef6c52a/build /usr/local/zig-0.13.0/zig /home/pierre/wrk/browser /home/pierre/wrk/browser/.zig-cache /home/pierre/.cache/zig --seed 0x3890d754 -Z932405236f7f5d84 run
|
@karlseguin I'm ready to merge the PR. Can you resolve the conflicts? I'm sorry, I merged others PRs which created them... thanks 🙏 |
|
ok, we had a small memory regression detected . I guess the test a bit too much restrictive. |
This includes / is built on top of #408
CDP is now a struct, with a cleaner separation between Server, Client and CDP. Server <-> Client <-> CDP -> Browser. The CDP instance is the state.
Optimized how cdp results and events are sent to the client - only a single allocation is now required.
Each CDP message is now processed with an arena, which is re-used for each message. Small ad-hoc message-specific allocations, like the
aux_datashould now be more efficient.Tried to remove some panics and undefines, or at least reduce the time that a field is undefined. Related, tried to remove the need for stable pointers (i.e.
Browser.initnow returns aBrowser, rather than needing to be given a*Browser).Improved parsing of CDP messages from the client.
Removed the newly added
browser.currentPage(), since the cdp already has directly access to the session. (browser.currentPage()would make it harder to implement the "TODO allow multiple sessions per browser.")Failed to get any unit test coverage from CDP, but i think it's important to do as there's more and more logic being added to it. Part of the blocker is global types. I'd consider introducing a global mock Browser/Session:
But I haven't given up hope on being able to quickly start a lightweight jsruntime.Env in unit tests.
To get a sense of the changes, I think you can look at
dom.discardSearchResultswhich shows:1 - how to get typed params
2 - how to access / modifity the state
3 - how to write a results