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

Migrate to Async NIOFileIO APIs #3167

Merged
merged 6 commits into from Apr 24, 2024
Merged

Migrate to Async NIOFileIO APIs #3167

merged 6 commits into from Apr 24, 2024

Conversation

0xTim
Copy link
Member

@0xTim 0xTim commented Apr 4, 2024

These changes are now available in 4.94.0

This migrates collectFile(at:) and writeFile(_:at:) to use NIO's async NIOFileIO APIs introduced in https://github.com/apple/swift-nio/releases/tag/2.63.0

Also adds a new API for streaming files using a AsyncSequence based on the new NIOFileSystem.

This work is required to move the DotEnv support over to an async API to avoid calling wait()s in an async context which can cause issues

@0xTim 0xTim added the semver-patch Internal changes only label Apr 4, 2024
@0xTim 0xTim requested a review from gwynne as a code owner April 4, 2024 00:36
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 29.41176% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 76.78%. Comparing base (11cdb29) to head (bae2a29).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3167      +/-   ##
==========================================
- Coverage   76.86%   76.78%   -0.09%     
==========================================
  Files         211      210       -1     
  Lines        8119     8132      +13     
==========================================
+ Hits         6241     6244       +3     
- Misses       1878     1888      +10     
Files Coverage Δ
Sources/Vapor/Utilities/FileIO.swift 71.42% <29.41%> (-4.33%) ⬇️

... and 1 file with indirect coverage changes

@0xTim 0xTim added semver-minor Contains new API and removed semver-patch Internal changes only labels Apr 17, 2024
Copy link
Member

@ptoffy ptoffy left a comment

Choose a reason for hiding this comment

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

Nothing blocking

/// - returns: `ByteBuffer` containing the file data.
public func collectFile(at path: String) async throws -> ByteBuffer {
guard
let attributes = try? FileManager.default.attributesOfItem(atPath: path),
Copy link
Member

Choose a reason for hiding this comment

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

While IMO this should use NIOFileSystem rather than FileManager I get that it probably makes sense to wait for it to be more stable

Sources/Vapor/Utilities/FileIO.swift Show resolved Hide resolved
0xTim and others added 6 commits April 24, 2024 01:26
* Add AsyncSequence-based file read method

* Add `NIOFileSystem` dependency to standard manifest

* Update test

* Add long test file
@0xTim 0xTim merged commit 4c80aab into main Apr 24, 2024
16 checks passed
@0xTim 0xTim deleted the async-fileio branch April 24, 2024 01:03
keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Apr 27, 2024
* main:
  Patch configuration and log actual port on startup (vapor#3160)
  Update provider tests to 5.10 (vapor#3178)
  Migrate to Async NIOFileIO APIs (vapor#3167)
  Removed streamFile deprecation + deactivated advancedETagComparison by default (vapor#3177)
  Remove HeadResponder (vapor#3147)
  Advanced ETag Comparison now supported (vapor#3015)
  Enabled Request Decompression By Default (vapor#3175)
  HTTP2 Response Compression/Request Decompression (vapor#3126)
  Don't set ignore status for SIGTERM and SIGINT on Linux (vapor#3174)
  Fix typos across the codebase (vapor#3162)
  Fix some Sendable warnings on 5.10 (vapor#3158)
  Allow `HTTPServer`'s configuration to be dynamically updatable (vapor#3132)
  Fix issue when client disconnects midway through a stream (vapor#3102)
  Fix handling of "flag" URL query params (vapor#3151)
  Bump the dependencies group with 1 update (vapor#3148)
  Merge Async Tests (vapor#3141)
  Fix URI handling with multiple slashes and variable components. (vapor#3143)
  Fix broken URI behaviors (vapor#3140)

# Conflicts:
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Contains new API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants