Skip to content

loop: add some comment on reset functions #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 9, 2025
Merged

loop: add some comment on reset functions #305

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88d29c9
loop: add some comment on reset functions
krichprollsch Apr 9, 2025
2c418d6
loop: inline cancelAll and remove useless resetEvents
krichprollsch Apr 9, 2025
9a7f1ea
loop: reset events before waiting during deinit
krichprollsch Apr 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/loop.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,12 @@ const log = std.log.scoped(.loop);
pub const SingleThreaded = struct {
alloc: std.mem.Allocator, // TODO: unmanaged version ?
io: IO,

// both events_nb are used to track how many callbacks are to be called.
// We use these counters to wait until all the events are finished.
js_events_nb: usize,
zig_events_nb: usize,

cbk_error: bool = false,

// js_ctx_id is incremented each time the loop is reset for JS.
Expand Down Expand Up @@ -284,21 +288,36 @@ pub const SingleThreaded = struct {
self.io.cancel_one(*ContextCancel, ctx, cancelCallback, completion, comp_cancel);
}

// cancelAll will cancel all future events.
// The loop will not be usable anymore after cancelAll.
// It is used only during the deinit.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's only used during deinit, inline it in deinit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, by inlining I realized resetEvents is even useless 🤦

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm wondering if we should call resetJS and resetZig before the deinit wait.
I think so b/c if the caller didn't we could be blocked by a callback re-creating a setTimeout for example.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean before the while (self.eventsNb(.js) > 0 or self.eventsNb(.zig) > 0) { in deinit?

I think this would also have solved the WPT crash. The Issue with the WPT crash was that wait/run wasn't called on an error (my fix was to add an errdefer js_env.wait()).

It isn't just about blocking forever, in the cast of WPT, the issue was that the callbacks are being executed in deinit after the env was cleared up.

// without `reset`, this will execute any pending callbacks, which references the destroyed env
defer loop.deinit()

// invalidates callbacks in the loop
defer js_env.deinit()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so yes, I think that's a good idea :)

fn cancelAll(self: *Self) void {
self.resetEvents(.js);
self.resetEvents(.zig);
self.io.cancel_all();
}

// Reset all existing JS callbacks.
// The existing events will happen and their memory will be cleanup but the
// corresponding callbacks will not be called.
pub fn resetJS(self: *Self) void {
self.js_ctx_id += 1;
self.cancelled.clearRetainingCapacity();
// We don't call self.resetEvents(.js) intentionnaly.
// Indeed we don't want to prevent the possibility to wait for events.
// The caller may want to wait to have memory correcly cleaned after
// the events happen, even if the callback are ignoreed.
}

// Reset all existing Zig callbacks.
// The existing events will happen and their memory will be cleanup but the
// corresponding callbacks will not be called.
pub fn resetZig(self: *Self) void {
self.zig_ctx_id += 1;
// We don't call self.resetEvents(.zig) intentionnaly.
// Indeed we don't want to prevent the possibility to wait for events.
// The caller may want to wait to have memory correcly cleaned after
// the events happen, even if the callback are ignoreed.
}

// IO callbacks APIs
Expand Down