Fixes bi-directional streaming#344
Uh oh!
There was an error while loading. Please reload this page.
Conversation
| break | ||
| case.head whereself.ignoreUncleanSSLShutdown, | ||
| .body whereself.ignoreUncleanSSLShutdown: | ||
| case.sending whereself.ignoreUncleanSSLShutdown: |
There was a problem hiding this comment.
test testNoResponseWithIgnoreErrorForSSLUncleanShutdown fails if I add .sent, I'm not sure why yet...
There was a problem hiding this comment.
.sending means that we have not sent client .end
There was a problem hiding this comment.
What's the difference between .sent and .endOrError? In many cases (content-length present) after having sent the response body, we're done?
There was a problem hiding this comment.
you are looking at slightly outdated code
There was a problem hiding this comment.
this is a slightly weird part, it tries to do the following: if ignoreUncleanSSLShutdown is set and we received response .head, then it's allowed to not fail
artemredkinforce-pushed the fix_bi_streaming branch 3 times, most recently from ce06b45 to 88f4dd3Compare
| }.flatMap{ | ||
| context.eventLoop.assertInEventLoop() | ||
| if case .endOrError =self.state { |
There was a problem hiding this comment.
could we replace this if case and the fallthrough by one full switch self.state?
It's unclear to me why we can move from almost any state to .sent and why all those transitions are legal.
There was a problem hiding this comment.
For example a transition from .sent to .sent seems odd, right? Or from .inactive to .sent seems wrong too.
| caseidle | ||
| casebodySent | ||
| caseinactive | ||
| casesending |
There was a problem hiding this comment.
I think we could come up with slightly more prescriptive names? What are wen sending here? The request, the response, the head, the body, the trailers? Some of them, all of them?
Same for sent
| privatefunc writeBodyPart(context:ChannelHandlerContext, part:IOData, promise:EventLoopPromise<Void>){ | ||
| switchself.state { | ||
| case.idle: | ||
| case.sending: |
There was a problem hiding this comment.
this seems a little dangerous? Previously we could send in .idle and now we can't?
There was a problem hiding this comment.
A full switch without the default would be better too here.
Uh oh!
There was an error while loading. Please reload this page.
| break | ||
| case.head whereself.ignoreUncleanSSLShutdown, | ||
| .body whereself.ignoreUncleanSSLShutdown: | ||
| case.sending whereself.ignoreUncleanSSLShutdown: |
There was a problem hiding this comment.
What's the difference between .sent and .endOrError? In many cases (content-length present) after having sent the response body, we're done?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
weissi left a comment
weissi left a comment There was a problem hiding this comment.
thank you! This looks like a great start, left some comments
| case sent | ||
| case head | ||
| case inactive | ||
| case sending_waiting |
There was a problem hiding this comment.
- we should use
lowerCamelCaseto match swift's usual style. - I think we can do better than
sendingWaiting, ie. what are we sending and what are we waiting for?
| case bodySent | ||
| case sent | ||
| case head | ||
| case inactive |
There was a problem hiding this comment.
idle is probably better than inactive because the channel is actually isActive = true, right?
artemredkinforce-pushed the fix_bi_streaming branch 2 times, most recently from 4b3c27d to 06a4098Compare
Lukasa left a comment
Lukasa left a comment There was a problem hiding this comment.
Seems reasonable enough to me, nothing obviously problematic in the code.
artemredkinforce-pushed the fix_bi_streaming branch from 06a4098 to f49c143CompareUh oh!
There was an error while loading. Please reload this page.
| case.sendingBodyResponseHeadReceived: | ||
| self.state =.bodySentResponseHeadReceived | ||
| case.bodySentWaitingResponseHead,.bodySentResponseHeadReceived: | ||
| preconditionFailure("should not happen") |
There was a problem hiding this comment.
should we print self.state in that message?
| self.state =.bodySentResponseHeadReceived | ||
| case.bodySentWaitingResponseHead,.bodySentResponseHeadReceived: | ||
| preconditionFailure("should not happen") | ||
| case.endOrError,.redirected: |
There was a problem hiding this comment.
mind adding a comment why that's okay in .endOrError, or .redirected? I don't quite get how we can be in .endbefore we get the end of the body.
There was a problem hiding this comment.
rewrote, added a test
Uh oh!
There was an error while loading. Please reload this page.
Motivation: When we stream request body, current implementation expects that body will finish streaming _before_ we start to receive response body parts. This is not correct, reponse body parts can start to arrive before we finish sending the request. Modifications: - Simplifies state machine, we only case about request being fully sent to prevent sending body parts after .end, but response state machine is mostly ignored and correct flow will be handled by NIOHTTP1 pipeline - Adds HTTPEchoHandler, that replies to each response body part - Adds bi-directional streaming test Result: Closesswift-server#327
artemredkinforce-pushed the fix_bi_streaming branch from f49c143 to 26b1191Compare
3 participants
Motivation:
When we stream request body, current implementation expects that body
will finish streaming before we start to receive response body parts.
This is not correct, reponse body parts can start to arrive before we
finish sending the request.
Modifications:
to prevent sending body parts after .end, but response state machine
is mostly ignored and correct flow will be handled by NIOHTTP1
pipeline
Result:
Closes#327