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

Async apis for NonBlockingFileIO #2576

Merged
merged 19 commits into from
Nov 16, 2023

Conversation

adam-fowler
Copy link
Contributor

@adam-fowler adam-fowler commented Oct 26, 2023

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

@adam-fowler
Copy link
Contributor Author

adam-fowler commented Oct 26, 2023

I think it is complaining because I've reduced the number of allocations for the readChunked integration test.

EDIT: I reduced MAX_ALLOCS_ALLOWED_read_10000_chunks_from_file to get integration tests working

@adam-fowler
Copy link
Contributor Author

I just saw #2578 which may clash with this PR as. I am passing NIOFileHandle across concurrency contexts. How should I proceed? I have used an @unchecked Sendable type to pass a FileRegion from the openFile operation. In this situation is it considered ok?

@FranzBusch
Copy link
Member

NIOFileHandle can definitely not become Sendable since they must be owned by a single isolation region at any given time. However, with the new region based isolation pitch a lot of the cases where you currently want NIOFileHandle to be Sendable should be solved.

@adam-fowler
Copy link
Contributor Author

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.

@dnadoba
Copy link
Member

dnadoba commented Oct 30, 2023

@adam-fowler You will need to use UnsafeTransfer in your implementation for this until we get region based isolation. You will need to copy the implementation over to NIOPosix as it is not public (and we don't want it to be public).

@adam-fowler
Copy link
Contributor Author

Any chance of a review here?

Copy link
Member

@FranzBusch FranzBusch left a 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.

Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@glbrntt glbrntt left a 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.

Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Copy link
Member

@FranzBusch FranzBusch left a 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

Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@glbrntt glbrntt 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 looks great, I left a couple of nits though.

Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Sources/NIOPosix/NonBlockingFileIO.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@dnadoba dnadoba left a 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.

@dnadoba
Copy link
Member

dnadoba commented Nov 16, 2023

If you feel fancy you could also change the byteCount parameter types from Int to Int64 and temporarily throw an error if the value doesn't fit exactly into an Int (i.e. Int(exactly: byteCount) returns nil) until we convert the underlying APIs to also take Int64. More infos in this issue comment: #2591 (comment)
It would be quite easy as we don't need to deprecate any API as the async API is new.

Note that I'm not suggesting to fix the 4GB Buffer overflow issue, just the type change from Int to Int64.
This can also be done in a separate PR and doesn't block this PR from being merged.

Copy link
Member

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

@adam-fowler
Copy link
Contributor Author

If you feel fancy you could also change the byteCount parameter types from Int to Int64 and temporarily throw an error if the value doesn't fit exactly into an Int (i.e. Int(exactly: byteCount) returns nil) until we convert the underlying APIs to also take Int64. More infos in this issue comment: #2591 (comment)
It would be quite easy as we don't need to deprecate any API as the async API is new.

@dnadoba I can add this. One minor issue though NonBlockingFileIO.Error is an enum which I can't extend. I'd have to add another error type. It might be better to deal with this in a separate PR which doesn't require the error because 64 bit read sizes will be supported.

@dnadoba
Copy link
Member

dnadoba commented Nov 16, 2023

Sure, we can just use a specific error that we later deprecate as this should only be a temporary issue I guess.

@dnadoba dnadoba merged commit e69354f into apple:main Nov 16, 2023
8 checks passed
@glbrntt glbrntt added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants