Skip to content
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

fsapi: migrates PollRead to Poll with Pflag #1599

Merged
merged 5 commits into from
Jul 30, 2023
Merged

Conversation

codefromthecrypt
Copy link
Contributor

This migrates PollRead to Poll so that it can support read, write or a combination via flags. This should allow poll_oneoff to be implemented even if internally it may need to use a deadline decrementing approach to cycle through each file instead of via multiple at the same time.

The benefit of doing things this way is we don't need to top-level a concept of file descriptor, which complicates the design of fsapi.FS which is far easier to implement if there are no "multi-file" apis on it.

This intentionally doesn't implement the approach at the same time, as the goal is to create the api to implement. Doing so also puts a lot less pressure on the task to complete migration off syscall types.

This migrates PollRead to Poll so that it can support read, write or a
combination via flags. This should allow `poll_oneoff` to be implemented
even if internally it may need to use a deadline decrementing approach
to cycle through each file instead of via multiple at the same time.

The benefit of doing things this way is we don't need to top-level a
concept of file descriptor, which complicates the design of `fsapi.FS`
which is far easier to implement if there are no "multi-file" apis on
it.

This intentionally doesn't implement the approach at the same time, as
the goal is to create the api to implement. Doing so also puts a lot
less pressure on the task to complete migration off syscall types.

Signed-off-by: Adrian Cole <[email protected]>
@@ -1,3 +1,5 @@
//go:build windows || linux || darwin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was missing. we really need to finish #1578 because it is very easy to break it more, with each change to things that can use syscalls.

Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
Copy link
Contributor

evacchi commented Jul 29, 2023

fixed the Windows build.

Without a lower-level concept of fd/handle, we improve encapsulation/portability but we won't be able to multiplex I/O (e.g. in this specific case, we won't be able to Poll([]{fd_1,...fd_n})). Probably being concerned with I/O performance is not worth it at this time (we are not really benchmarking I/O), but it's worth noting. Maybe something to add to RATIONALE.md (e.g. "why we encapsulate FDs" or something?)

@codefromthecrypt
Copy link
Contributor Author

@evacchi I think by "why we encapsulate FDs" what you are asking is why do we expose a replacement for fs.FS/fs.File?

The primary use case of this abstraction is to be able to write delete, etc files. Currently, we continually break portability and if we don't stop doing that we can never expose it. I'll make notes on this.

@codefromthecrypt
Copy link
Contributor Author

Also, it would be helpful when discussing what won't work, is to be very concrete. For example, some existing case (real not forged, like test we are failing) that cannot be solved as a poll loop or as a goroutine externally. We have a tendency to over-solve a lot and by making failing tests it helps conserve energy.

Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Contributor Author

@mathetake can you take a look? I had planned to do code and still have time today. I spent the last 3 hours explaining why our abstraction doesn't need to up front interfaces for every niche features, using multi-poll as an example of that.

While some use cases are solved with real files, not all are. Regardless, an
interface approach is necessary to ensure users can intercept I/O operations.

### Why doesn't `sys.File` have a `Fd()` method?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: I'm using sys.File as even though we may have forgotten, we did promise to expose our filesystem abstraction in the next release. Even if that ends up experimentalsys.File, the end name would be this.

Adrian Cole added 2 commits July 30, 2023 10:32
Signed-off-by: Adrian Cole <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt codefromthecrypt merged commit 8d3874d into main Jul 30, 2023
59 checks passed
@codefromthecrypt codefromthecrypt deleted the pollread-poll branch July 30, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants