Only forward bytes on removal when we know we can handle it #430
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@normanmaurer doesn't compile, probably forgot to record something |
normanmaurer
force-pushed
the
forward_bytes_only_if_configured
branch
from
62a6e5e
to
37cc0c1
Compare
|
@weissi lol yeah... stupid me :( PTAL again |
|
@Lukasa if you prefer an enum please let me know and also suggest a name ;) |
Lukasa
requested changes
May 23, 2018
Sources/NIOHTTP1/HTTPDecoder.swift
Outdated
| /// - parameters: | ||
| /// - forwardAfterUpgrade: if `true` any remaining bytes that are currently buffered by the `HTTPRequestDecoder` will be forwarded | ||
| /// to the next handler in the pipeline once this decoder is removed from the pipeline and an upgrade was detected. | ||
| public convenience init(forwardAfterUpgrade: Bool) { |
There was a problem hiding this comment.
Let's have an enum.
enum RemoveAfterUpgradeStrategy {
case forwardBytes
case dropBytes
}
normanmaurer
force-pushed
the
forward_bytes_only_if_configured
branch
from
37cc0c1
to
0b13de6
Compare
|
@Lukasa PTAL again... using an enum now. |
Lukasa
requested changes
May 23, 2018
Sources/NIOHTTP1/HTTPDecoder.swift
Outdated
| @@ -187,6 +195,14 @@ public final class HTTPResponseDecoder: HTTPDecoder<HTTPClientResponsePart>, Cha | |||
| } | |||
| } | |||
|
|
|||
| /// Strategy to use when a decoder is removed from the pipeline and an HTTP upgrade was detected before. | |||
There was a problem hiding this comment.
Nit: let's write this as "Strategy to use when a HTTPDecoder is removed from a pipeline after a HTTP upgrade was detected".
There was a problem hiding this comment.
addressed... will merge once green if I not hear back :)
normanmaurer
force-pushed
the
forward_bytes_only_if_configured
branch
from
0b13de6
to
6a34899
Compare
Lukasa
approved these changes
May 23, 2018
Lukasa
added
the
🔼 needs-minor-version-bump
Motivation: 398b950 introduced a change which forwarded bytes that were buffered in HTTPDecoder after it was removed when an upgrade was detected. This may be risky if we dont know that the pipeline can handle it. Modifications: - Add new initializer that allow us to configure if we should forward bytes or not. - Automatically use this if we know that we added an upgrader - Add unit tests. Result: Less risky implementation.
normanmaurer
force-pushed
the
forward_bytes_only_if_configured
branch
from
fab0a12
to
b88ce7f
Compare
weissi
added
patch-version-bump-only
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
398b950 introduced a change which forwarded bytes that were buffered in HTTPDecoder after it was removed when an upgrade was detected. This may be risky if we dont know that the pipeline can handle it.
Modifications:
Result:
Less risky implementation.