loop: add some comment on reset functions #305
Conversation
src/loop.zig
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
If it's only used during deinit, inline it in deinit?
There was a problem hiding this comment.
Good point, by inlining I realized resetEvents is even useless 🤦
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
so yes, I think that's a good idea :)
No description provided.