-
Notifications
You must be signed in to change notification settings - Fork 651
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
Async apis for NonBlockingFileIO #2576
Conversation
I think it is complaining because I've reduced the number of allocations for the readChunked integration test. EDIT: I reduced |
I just saw #2578 which may clash with this PR as. I am passing |
|
Oh I know it shouldn't be Sendable. But what about the situation this PR covers where it is created on a NIOThreadPool and immediately passed back to a Task. |
@adam-fowler You will need to use |
Any chance of a review here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good to me. I would like @glbrntt to also take a look since he spent some time with file systems recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, left couple of comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some last comments but looks really good
dd41786
to
c93ce06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looks great, I left a couple of nits though.
I have not produced async versions of the readChunked functions. These would likely be replaced with an AsyncSequence though.
Use in NonBlockingFileIO.openFile functions
Co-authored-by: George Barnett <[email protected]>
Also - move @available above each function - Remove wording about separate from Task pool
Co-authored-by: George Barnett <[email protected]>
09763c5
to
85deeb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are planning to cut a release in the next couple hours. Let's wait til we merge this until then.
If you feel fancy you could also change the Note that I'm not suggesting to fix the 4GB Buffer overflow issue, just the type change from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dnadoba I can add this. One minor issue though |
Sure, we can just use a specific error that we later deprecate as this should only be a temporary issue I guess. |
Provide async versions of most NonBlockingFileIO APIs. This PR provides versions of all functions apart from the
readChunked
function which should probably be replaced by an AsyncSequence.Some of the code running on the NIOThreadPool that would have been duplicated has been broken out into separate functions so it can be used by both ELF and async functions.
I've also provided async versions of the relevant tests