Skip to content

Make os.zig not depend on the event loop #6412

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.

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 14 commits into from
Sep 25, 2020
Merged

Make os.zig not depend on the event loop #6412

merged 14 commits into from
Sep 25, 2020

Conversation

kristoff-it
Copy link
Member

@kristoff-it kristoff-it commented Sep 24, 2020

This is #5620, I force-pushed and it broke the PR.

Currently os uses the event loop when receiving EAGAIN, which doesn't allow us to support completion APIs like io_uring. With this PR, now it's fs and net that (in async mode) delegate the whole I/O operation to loop.zig, and it's loop that can then decide to do the readyness dance with EAGAIN, or (once someone implements it) use io_uring or equivalents.

Read the comments in #5620 for more details about sendfile and open design questions.

Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
Signed-off-by: Loris Cro <[email protected]>
@alexnask alexnask added the standard library This issue involves writing Zig code for the standard library. label Sep 24, 2020
@kubkon
Copy link
Member

kubkon commented Sep 24, 2020

Re Windows CI erroring out, I think the error code of 3221226505 that we might see, and/or the error message LLVM ERROR: out of memory are correlated with Azure live site incident currently ongoing. This is me just guessing right now since I can't replicate any of those on my local Win install, not to mention the fact that #6397 which had changes pushed today passed Win CI just fine.

@FireFox317
Copy link
Contributor

FireFox317 commented Sep 24, 2020

One thing to consider is that the semantics of the functions in std.os are not valid anymore when using async io, because then an error (WouldBlock) will be returned, while in reality there really is no error.

However it is significantly better to not have any event loop code in the std.os functions.

@kristoff-it
Copy link
Member Author

@FireFox317 yes, the idea is that if you're using high-level APIs (i.e. fs and net), you can continue doing so and nothing should change, while if you want to use lower-level APIs, then you have to either call into the event loop or call into os but then handle WoulBlock manually.

@andrewrk
Copy link
Member

Don't forget the doc comments in os.zig

Signed-off-by: Loris Cro <[email protected]>
@kristoff-it
Copy link
Member Author

Docs updated and it didn't break the build (you never know :D)

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

This is ready to land. I have 1 little fixup for you that doesn't actually change the behavior.

Co-authored-by: Andrew Kelley <[email protected]>
@andrewrk andrewrk merged commit a502604 into ziglang:master Sep 25, 2020
@@ -720,6 +720,50 @@ pub const Loop = struct {
}
}

/// ------- I/0 APIs -------
Copy link
Contributor

@codehz codehz Sep 29, 2020

Choose a reason for hiding this comment

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

maybe typo? I/0 vs I/O

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants