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

Add support for asynchronous body stream writing #2998

Merged
merged 21 commits into from Apr 30, 2024
Merged

Add support for asynchronous body stream writing #2998

merged 21 commits into from Apr 30, 2024
+176 −2

Conversation

Joannis
Copy link
Member

@Joannis Joannis commented Apr 14, 2023 •
edited by penny-for-vapor bot

These changes are now available in 4.95.0

  • Fixes Crash during upload  #2930 - a crash when users try to write a body from within a task towards the ELF APIs.
  • Introduces a new API for writing chunked HTTP response bodies
  • Adds a helper that automatically manages failing and closing streams

Joannis and others added 15 commits January 19, 2023 13:47
@Joannis
Add support for asynchronous body stream writing, thereby also fixing…
3c97f93 
… crashes when users try to write a body from within a task towards the ELF APIs
@Joannis
Fix compilation for Swift 5.6 inference
875881f
@Joannis
Add support for asynchronous body stream writing, thereby also fixing…
fa426b9 
… crashes when users try to write a body from within a task towards the ELF APIs
@Joannis
Fix compilation for Swift 5.6 inference
c672309
@Joannis
Fix older swift compilign tests
d300db7
@Joannis
Merge branch 'feature/jo-async-body-stream' of github.com:vapor/vapor…
d04d1ec 
… into feature/jo-async-body-stream

# Conflicts:
#	Tests/VaporTests/PipelineTests.swift
@gwynne
Merge branch 'main' into feature/jo-async-body-stream
11bdbba
@Joannis
Fix the test, as you cannot write a response while the request is sti…
6efc1c0 
…ll inbound
@0xTim
Merge branch 'main' into feature/jo-async-body-stream
afbeaf2
@Joannis
Add docc comments, remove TODOs and converted them into an issue
19e4706
@Joannis
Merge remote-tracking branch 'origin/main' into feature/jo-async-body…
9629929 
…-stream
@Joannis
Be explicit in the return tpe that fails to infer on Swift 5.6
6b5764e
@0xTim
Merge branch 'main' into feature/jo-async-body-stream
168b822
@Joannis
Merge remote-tracking branch 'origin/main' into feature/jo-async-body…
fdc3c9f 
…-stream

# Conflicts:
#	Sources/Vapor/Concurrency/Request+Concurrency.swift
@Joannis
Remove Swift 5.6 support
204b047
@Joannis Joannis requested a review from gwynne April 14, 2023 07:27
@grahamburgsma
Copy link
Contributor

Considering this was already approved in #2939 by @0xTim and Swift 5.6 support has been removed, could this be moved forward?

@makaGeorge
Copy link

Any news on this? Thanks!

@3a4oT
Copy link
Contributor

3a4oT commented Oct 24, 2023

Hey guys, can we proceed with it? cc @0xTim

@0xTim
Copy link
Member

0xTim commented Oct 24, 2023

@3a4oT I need to finish making Request Sendable first before we can merge this in and get it working with confidence. Response has just been finished so Request is last

@tareksabry1337
Copy link

With both Request / Response now being Sendable any chance this + #2960 will see the light soon? We are eagerly waiting for SSE

@0xTim
Copy link
Member

0xTim commented Nov 11, 2023

@tareksabry1337 yes they are on the list. The todo list currently looks like

  1. fix client disconnect issue for streaming request bodies
  2. fix websocket crash
  3. This

@grahamburgsma grahamburgsma mentioned this pull request Feb 15, 2024
Open
@Joannis Joannis requested a review from 0xTim as a code owner April 30, 2024 01:00
0xTim
0xTim approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

I've updated the PR so we can merge it and integrated it with all the recent Sendable work. Will let @gwynne take a look as well

gwynne
gwynne approved these changes Apr 30, 2024
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.

Looks okay to me at this point.

@0xTim 0xTim added the semver-minor Contains new API label Apr 30, 2024
@0xTim 0xTim enabled auto-merge (squash) April 30, 2024 11:29
@0xTim 0xTim merged commit c524f60 into main Apr 30, 2024
15 of 16 checks passed
@0xTim 0xTim deleted the feature/jo-async-body-stream branch April 30, 2024 11:35
This was referenced Apr 30, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gwynne gwynne gwynne approved these changes

@0xTim 0xTim 0xTim 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.

Crash during upload
7 participants
@Joannis @grahamburgsma @makaGeorge @3a4oT @0xTim @tareksabry1337 @gwynne