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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement new streamwriter api #291

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

artemredkin
Copy link
Collaborator

@artemredkin artemredkin commented Aug 6, 2020

馃毀 This is slightly WIP.
Revamps StreamWriter.

Motivation:
There are two issues with current StreamWriter API:

  1. no access to allocator
  2. writer completion is super confusing, it will be considered finished when the returned EventLoopFuture is completed, which is not ideal.

Modifications:

  1. Deprecate old StreamWriter API
  2. Introduce new type StreamWriter2 (really need help with the naming here) with the following API:
let body: HTTPClient.Body = .stream2(length: 8) { writer in
    writer.write(writer.allocator.buffer(string: "1234").whenComplete { _ in
        writer.write(writer.allocator.buffer(string: "1234").whenComplete { _ in
            writer.end()
        }
    }
}

Result:
Closes #194
Closes #264

@artemredkin artemredkin added this to the 1.2.0 milestone Aug 6, 2020
@artemredkin artemredkin force-pushed the revamp_stream_writer branch 2 times, most recently from 5924d12 to 0cb498a Compare August 6, 2020 16:09
/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
/// unless `Transfer-Encoding: chunked` header is set.
/// - stream: Body chunk provider.
public static func stream2(length: Int? = nil, _ stream: @escaping (StreamWriter2) -> Void) -> Body {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I we add method with the same name but different type we will break compilation, because some calls could become ambiguous 馃槩

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that there is discussion about possibly breaking API in the nearish future anyway, we may find we want to delay this until then to resolve this problem.

public struct StreamWriter2 {
public let allocator: ByteBufferAllocator
let onChunk: (IOData) -> EventLoopFuture<Void>
let onComplete: EventLoopPromise<Void>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we're writing this as a callback-taking type rather than just holding onto the task object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and "forward" all write requests to it's channel? that might be a good idea, will try that, thanks!

/// - length: Body size. Request validation will be failed with `HTTPClientErrors.contentLengthMissing` if nil,
/// unless `Transfer-Encoding: chunked` header is set.
/// - stream: Body chunk provider.
public static func stream2(length: Int? = nil, _ stream: @escaping (StreamWriter2) -> Void) -> Body {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that there is discussion about possibly breaking API in the nearish future anyway, we may find we want to delay this until then to resolve this problem.

@ktoso ktoso changed the base branch from master to main August 20, 2020 01:31
@ktoso
Copy link
Contributor

ktoso commented Aug 20, 2020

Just as a heads up, the main development branch has been changed to main, following the Swift policy on this.

This PR has been re-targeted to main and should just work. However when performing rebases etc please keep this in mind -- you may want to fetch the main branch and rebase onto the main branch from now on since master is not up-to-date anymore and is going to be deleted shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants