mirror of
https://github.com/lightpanda-io/browser.git
synced 2025-10-29 23:23:28 +00:00
Rework internal navigation to prevent deadlocking
The mix of sync and async HTTP requests requires care to avoid deadlocks.
Previously, it was possible for async requests to use up all available HTTP
state objects duration a navigation flow (either directly, or via an internal
redirect (e.g. click, submit, ...)). This would block the navigation, which,
because everything is single thread, would block the I/O loop, resulting in a
deadlock.
The correct solution seems to be to remove all synchronous I/O. And I tried to
do that, but I ran into a wall with module-loading, which is initiated from V8.
V8 says "give me the source for this module", and I don't see a great way to
tell it: wait a bit.
So I went back to trying to make this work with the hybrid model, despite last
weeks failures to get it to work. I changed two things:
1 - The http client will only directly initiate an async request if there's
at least 2 free state objects available (1 for the request, and leaving 1
free for any synchronous requests)
2 - Delayed navigation retries until there's at least 1 free http state object
available.
Commits from last week did help with this. First, we're now guaranteed to have
a single sync-request at a time (previously, we could have had 2). Secondly,
the async connection is now async end-to-end (previously, it could have blocked
on an empty state pool).
We could probably make this a bit more obviously by reserving 1 state object
for synchronous requests. But, since the long term solution is probably having
no synchronous requests, I'm happy with anything that lets me move past this
issue.
This commit is contained in:
@@ -22,6 +22,7 @@ const Allocator = std.mem.Allocator;
|
||||
|
||||
const Env = @import("env.zig").Env;
|
||||
const Page = @import("page.zig").Page;
|
||||
const URL = @import("../url.zig").URL;
|
||||
const Browser = @import("browser.zig").Browser;
|
||||
const NavigateOpts = @import("page.zig").NavigateOpts;
|
||||
|
||||
@@ -72,7 +73,7 @@ pub const Session = struct {
|
||||
|
||||
pub fn deinit(self: *Session) void {
|
||||
if (self.page != null) {
|
||||
self.removePage() catch {};
|
||||
self.removePage();
|
||||
}
|
||||
self.cookie_jar.deinit();
|
||||
self.storage_shed.deinit();
|
||||
@@ -104,7 +105,7 @@ pub const Session = struct {
|
||||
return page;
|
||||
}
|
||||
|
||||
pub fn removePage(self: *Session) !void {
|
||||
pub fn removePage(self: *Session) void {
|
||||
// Inform CDP the page is going to be removed, allowing other worlds to remove themselves before the main one
|
||||
self.browser.notification.dispatch(.page_remove, .{});
|
||||
|
||||
@@ -127,12 +128,6 @@ pub const Session = struct {
|
||||
// window.setTimeout and running microtasks should be ignored
|
||||
self.browser.app.loop.reset();
|
||||
|
||||
// Finally, we run the loop. Because of the reset just above, this will
|
||||
// ignore any timeouts. And, because of the endScope about this, it
|
||||
// should ensure that the http requests detect the shutdown socket and
|
||||
// release their resources.
|
||||
try self.browser.app.loop.run();
|
||||
|
||||
self.page = null;
|
||||
|
||||
// clear netsurf memory arena.
|
||||
@@ -144,28 +139,4 @@ pub const Session = struct {
|
||||
pub fn currentPage(self: *Session) ?*Page {
|
||||
return &(self.page orelse return null);
|
||||
}
|
||||
|
||||
pub fn pageNavigate(self: *Session, url_string: []const u8, opts: NavigateOpts) !void {
|
||||
// currently, this is only called from the page, so let's hope
|
||||
// it isn't null!
|
||||
std.debug.assert(self.page != null);
|
||||
|
||||
defer if (self.page) |*p| {
|
||||
if (!p.delayed_navigation) {
|
||||
// If, while loading the page, we intend to navigate to another
|
||||
// page, then we need to keep the transfer_arena around, as this
|
||||
// sub-navigation is probably using it.
|
||||
_ = self.browser.transfer_arena.reset(.{ .retain_with_limit = 64 * 1024 });
|
||||
}
|
||||
};
|
||||
|
||||
// it's safe to use the transfer arena here, because the page will
|
||||
// eventually clone the URL using its own page_arena (after it gets
|
||||
// the final URL, possibly following redirects)
|
||||
const url = try self.page.?.url.resolve(self.transfer_arena, url_string);
|
||||
|
||||
try self.removePage();
|
||||
var page = try self.createPage();
|
||||
return page.navigate(url, opts);
|
||||
}
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user