apple / swift-nio

Event-driven network application framework for high performance protocol servers & clients, non-blocking.
https://swiftpackageindex.com/apple/swift-nio/documentation
Apache License 2.0
8k stars 652 forks source link

it's too hard to make HTTP/1.1 pipelining work correctly, we should offer a solution #43

Closed weissi closed 6 years ago

weissi commented 6 years ago

At the moment, it's too hard to make HTTP/1.1 pipelining work correctly. SwiftNIO just forwards you the HTTP request parts as they come in and the user needs to make sure everything works correctly with pipelining which is hard (even our example NIOHTTP1Server doesn't do that (see #9)).

We should offer a HTTP1PipeliningHandler which forwards the n-th request only if the (n-1)th request has been completed. That way the user can basically forget about pipelining which is a good thing :)

helje5 commented 6 years ago

This should probably be a blocking bug for the 1.0 release because the implications are so bad. Oh, wait ;-)

weissi commented 6 years ago

@helje5 1.0.0 for us doesn't mean it's complete by any means. It's just for SemVer purposes and we tagged 1.0.0 on release day. The only thing 1.0.0 means is that we intend to make the version 2.0.0 whenever we break existing API. Currently I think we're at 1.1.0 and also PR #62 addresses this issue which will likely be released as 1.2.0. Makes sense?

helje5 commented 6 years ago

Hey, you call Swift 4, Swift 4, so I think expectations are set quite right :-)

BTW: If I'm not mistaken PR #62 is a SemVer breaking change (adding a parameter to a function in a base type), even if it is unlikely that it actually break something.

Lukasa commented 6 years ago

BTW: If I'm not mistaken PR #62 is a SemVer breaking change (adding a parameter to a function in a base type), even if it is unlikely that it actually break something.

I don't think so, at least not meaningfully. ChannelPipeline is a final class, so there cannot be an override that is requiring this method to have a specific signature. The only thing we could plausibly collide with is an extension defined by a third-party that happened to have the signature. However, if we say that the requirement of SemVer minor is that we don't ever collide with a method that any user may hypothetically have defined then there is no such thing as a SemVer minor change in Swift:

For that reason, I think we have to say that we don't defend against the possibility that a user may have defined a colliding method in an extension in our SemVer, so this is still SemVer minor.

helje5 commented 6 years ago

I don't want to educate you about SemVer, but you are very right in that Swift is super fragile wrt it. (I think it is best if you would call it "SwiVer" and then outline the Swift features the API consumer is not allowed to use, say adding an extension to an API class.) Do I think that this is "meaningful" right now? No, there is no adoption yet, so you don't really break anyone :-) It would have been better to not call it a 1.0.0 and thereby wasting a SemVer block for something which isn't actually API stable ...

So what would be a SemVer compatible approach to add something like that? In the specific situation I think this would be sufficient as extensions are also scoped and statically dispatched:

public extension PipelineThing {
  public func addHTTPServerHandlers(enablePipelining: Bool) {
    ... do great stuff ...
  }
}

But hey, your project, do whatever you like! ¯_(ツ)_/¯

P.S.: A good way to think about this is, is to consider ABI. If I would link my server written against 1.1 to a 1.2 package, it would just break.

Lukasa commented 6 years ago

It would have been better to not call it a 1.0.0 and thereby wasting a SemVer block for something which isn't actually API stable ...

Version numbers are free and in functionally infinite supply. We didn't really waste anything. :smile: More importantly, SwiftPM works a lot better if you go to 1.0.0 early (e.g. from: behaves sensibly).

So what would be a SemVer compatible approach to add something like that?

That has very similar problems: if such an extension already exists in user code, the Swift compiler will complain that it doesn't know which method it wants to call.

Your ABI note is definitely well taken, and when stable ABI comes around we'll have a much clearer target for what SemVer applies to. Until that time, however, we've had to land on something that gives SemVer some semblance of reasonableness.

helje5 commented 6 years ago

Version numbers are free and in functionally infinite supply

In response I can only refer to SemVer:

If even the tiniest backwards incompatible changes to the public API require a major version bump, won’t I end up at version 42.0.0 very rapidly? This is a question of responsible development and foresight. Incompatible changes should not be introduced lightly to software that has a lot of dependent code. The cost that must be incurred to upgrade can be significant. Having to bump major versions to release incompatible changes means you’ll think through the impact of your changes, and evaluate the cost/benefit ratio involved.

🤷

So what would be a SemVer compatible approach to add something like that? (extension)

That has very similar problems: if such an extension already exists in user code, the Swift compiler will complain that it doesn't know which method it wants to call.

Did you try it? I tried it. As a matter of fact there is no such complain. If the compiler sees a symbol, it always picks the one from the current module (as you would expect). No ambiguity to see here. The same goes for global functions or other types: TestScoping.zip

You can version however you like, that is absolutely fine. But it is very precisely defined what SemVer is and means, their FAQ is great. You can argue circles around that, but you can't change what it is or make it "more reasonable". Just don't call it SemVer :-)

As it will probably become quite popular, it is important that consumers of your code understand how you version and what Swift features they are allowed to use. That doesn't have to be SemVer! You could very well state that users need to prepared to see new methods in classes and be aware of the implications for extensions and such (e.g. prefix them).

I would like to close this thread with another quote from the SemVer FAQ:

As soon as you realize that you’ve broken the Semantic Versioning spec, fix the problem and release a new minor version that corrects the problem and restores backwards compatibility

🤦‍♀️

Lukasa commented 6 years ago

In response I can only refer to SemVer:

Sure, I'm in agreement. However, I see no reason to be afraid of breaking changes, just to take them seriously. As you'll note, we have currently left breaking changes pending in our open PR's, without merging them. We aren't revving or breaking without reason. We're just also not tying ourselves to year-long release cycles at this early stage.

Did you try it? I tried it. As a matter of fact there is no such complain.

Awesome, then we're fine! You inspired me to test this, and in fact for a final class there is no misbehaviour by making this change. In all cases if there is a matching extension in the local module it will be preferred, otherwise the one we ship will be. No breaking, the patch is (API-wise) SemVer minor.

As you note, there are concerns about whether SemVer applies to ABI changes, and in a world where Swift had a stable ABI this change would definitely be SemVer major. However, it's not, and I see no reason to worry about it at this time. Given that SemVer expressly does not discuss ABI, I'm pretty happy saying that for the moment we care about API breakage, not hypothetical ABI breakage. Come the release of a stable ABI for Swift, we should re-evaluate, and would love your input at that time.

helje5 commented 6 years ago

in fact for a final class there is no misbehaviour by making this change

There is no misbehaviour for any class, final or not. Non-protocol Swift extensions are always statically dispatched, they are not Objective-C categories. (in fact that is the whole point, avoid the issues categories have when they are consumed by a different module)

Lukasa commented 6 years ago

Resolved by #62.