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.

Sign up for GitHub

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

Jump to bottom

More Sendable Conformances #3057

Merged
merged 37 commits into from
Sep 26, 2023
Merged

More Sendable Conformances #3057

merged 37 commits into from
Sep 26, 2023

Conversation

0xTim
Copy link
Member

@0xTim 0xTim commented Aug 30, 2023

This adds more Sendable conformances to Vapor and resolves any Sendable warnings that aren't related to Request or Response

@0xTim 0xTim marked this pull request as ready for review August 30, 2023 12:32
@0xTim 0xTim requested a review from gwynne as a code owner August 30, 2023 12:32
@0xTim 0xTim added the semver-minor Contains new API label Aug 30, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023 •
edited

Codecov Report

Merging #3057 (6235eec) into main (a4b0715) will increase coverage by 0.19%.
Report is 2 commits behind head on main.
The diff coverage is 92.45%.

❗ Current head 6235eec differs from pull request most recent head d3d915b. Consider uploading reports for the commit d3d915b to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3057      +/-   ##
==========================================
+ Coverage   76.21%   76.40%   +0.19%     
==========================================
  Files         211      211              
  Lines        7821     7893      +72     
==========================================
+ Hits         5961     6031      +70     
- Misses       1860     1862       +2
Files Changed Coverage Δ
...rces/Vapor/Authentication/BasicAuthorization.swift 59.25% <ø> (ø)
...ces/Vapor/Authentication/BearerAuthorization.swift 58.82% <ø> (ø)
...apor/Concurrency/ResponseCodable+Concurrency.swift 37.83% <ø> (ø)
Sources/Vapor/HTTP/BodyStream.swift 19.04% <ø> (ø)
Sources/Vapor/HTTP/Headers/HTTPHeaders+Name.swift 74.07% <ø> (ø)
Sources/Vapor/Utilities/File.swift 88.46% <ø> (ø)
...s/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift 91.93% <66.66%> (-0.75%) ⬇️
Sources/Vapor/Response/Response+Body.swift 66.03% <83.33%> (+0.01%) ⬆️
Sources/Vapor/Utilities/FileIO.swift 75.60% <84.37%> (+0.60%) ⬆️
Sources/Vapor/HTTP/Server/HTTPServer.swift 82.73% <93.33%> (+1.81%) ⬆️
... and 6 more

... and 1 file with indirect coverage changes

@0xTim
Copy link
Member Author

0xTim commented Aug 31, 2023

@fabianfett @FranzBusch if you'd like to have a look that would be great! This has more changes than simply wrapping properties in NIOLockedValueBox as it touches parts of NIO that can't be Sendable

gwynne
gwynne requested changes Sep 6, 2023
View reviewed changes
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Definitely shaping up, but several additional nits.

Sources/Vapor/HTTP/Server/HTTPServerResponseEncoder.swift Outdated Show resolved Hide resolved
Sources/Vapor/Request/Request+BodyStream.swift Outdated Show resolved Hide resolved
Sources/Vapor/Response/Response+Body.swift Outdated Show resolved Hide resolved
Sources/Vapor/HTTP/Server/HTTPServerResponseEncoder.swift Outdated Show resolved Hide resolved
Sources/Vapor/HTTP/Server/HTTPServerResponseEncoder.swift Outdated Show resolved Hide resolved
Sources/Vapor/Response/Response+Body.swift Outdated Show resolved Hide resolved
@0xTim 0xTim mentioned this pull request Sep 9, 2023
Closed
@0xTim 0xTim linked an issue Sep 9, 2023 that may be closed by this pull request
It seems comments reference HTTPResponse, but the actual type is Response #3067
Closed
@0xTim 0xTim requested a review from gwynne September 9, 2023 15:38
@0xTim 0xTim requested a review from gwynne September 21, 2023 13:21
gwynne
gwynne requested changes Sep 22, 2023
View reviewed changes
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

A couple bits and pieces, and one "hey if this is correct (and you should make sure I'm right before doing anything about it) it blows the whole PR out of the water" concern 😅

Package.swift Show resolved Hide resolved
Package.swift Show resolved Hide resolved
Sources/Vapor/Utilities/FileIO.swift Show resolved Hide resolved
Tests/VaporTests/Utilities/CapturingMetricsSystem.swift Outdated Show resolved Hide resolved
Tests/VaporTests/Utilities/CapturingMetricsSystem.swift Outdated Show resolved Hide resolved
fred-sch and others added 5 commits September 22, 2023 16:15
@fred-sch @0xTim @gwynne
HTTPServerErrorHandler: Improve Error Handling for HTTPParserError (#…
f325e76 
…2922)

* fix: HTTPServerErrorHandler catch HTTPParserError
Refs: #2921
* add reference to https://github.com/apple/swift-nio/blob/00341c92770e0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift
* set hasUnterminatedResponse in write
* error-handler needs to be before response encoder
* add test for invalid http
* rephrase and improve comments slightly
* remove http1 error handler from http2 pipeline
---------

Co-authored-by: Tim Condon <0xTim@users.noreply.github.com>
Co-authored-by: Gwynne Raskind <gwynne@vapor.codes>
@0xTim
Fix mismatching manifests
a48c042
@0xTim
PR feedback
35aa1a9
@0xTim
Merge branch 'main' into sendable-2
ba2b60d
@0xTim
Trigger CodeQL
6235eec
gwynne
gwynne approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

I can't claim I find this PR to be a masterpiece of elegance and precision, but preconcurrency is preconcurrency, and as far as I can tell there aren't any actual issues left.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@0xTim 0xTim merged commit 446b877 into main Sep 26, 2023
15 of 16 checks passed
@0xTim 0xTim deleted the sendable-2 branch September 26, 2023 12:07
@penny-for-vapor
Copy link

These changes are now available in 4.84.0

@Lukasa Lukasa mentioned this pull request Oct 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianfett fabianfett fabianfett left review comments

@gwynne gwynne gwynne approved these changes

Assignees
No one assigned
Labels
semver-minor Contains new API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

It seems comments reference HTTPResponse, but the actual type is Response
5 participants
@0xTim @codecov-commenter @gwynne @fabianfett @fred-sch