-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
HTTP2 and gzip #1188
HTTP2 and gzip #1188
Conversation
TODO: OpenSSL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
Sources/Async/Streams/Stream.swift
Outdated
@@ -0,0 +1,80 @@ | |||
/// A stream is both an InputStream and an OutputStream | |||
public typealias Stream = InputStream & OutputStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async module should be removed.
import Async | ||
import Bits | ||
|
||
public final class BodyStream: Async.Stream, ClosableStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments
import Bits | ||
import Foundation | ||
|
||
final class ChunkEncoder: Async.Stream, ClosableStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments
stream.onClose(onClose) | ||
} | ||
|
||
/// - TODO: Improve performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make that a private comment
|
||
// Send the settings, next | ||
client.updateSettings(to: settings) | ||
}.catch(client.promise.fail) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
extension HTTP2Client { | ||
/// Updates the frame's contents as settings, sends an acknowledgement | ||
/// | ||
/// TODO: Settings changed in the middle of the stream affecting the context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have these as //
so they don't show up in the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They won't, since they're internal APIs. And they serve as a good reminder
] | ||
|
||
func testClient() throws { | ||
// #if os(Linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get these working before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have an HTTP/2 host that I can test again. External hosts like google.com can not be connected to from CircleCI
XCTAssertThrowsError(try parser.parse(from: data)) | ||
} | ||
|
||
func testTooLargeResponse() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Plug some small Vapor 3 memory leaks
This PR will focus on HTTP2 and Gzip.
The reason this is bundled is because HTTP2 relies on Huffman trees, which are are one of the primary components of Gzip. Gzip will not implement a good algorithm such as LZ77 yet but will instead use a primitive and unperformant implementation as a placeholder for a future PR.
Current state:
More todos will be added once they're almost up for development...