notify delegate about connect errors#245
Conversation
| let delegate = TestDelegate() | ||
| let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") | ||
| do { | ||
| try httpBin.shutdown() |
There was a problem hiding this comment.
this should be in an XCTAssertNoThrow
| let request = try HTTPClient.Request(url: "http://localhost:\(httpBin.port)/get") | ||
| do { | ||
| try httpBin.shutdown() | ||
| _ = try httpClient.execute(request: request, delegate: delegate).wait() |
There was a problem hiding this comment.
please use
XCTThrowsError(try httpClient.execute(...).wait()) { error in
XCTAssert(...)
}
we literally had security issues before because of this pattern.
There was a problem hiding this comment.
done, thank you for catching this!
weissi
left a comment
weissi
left a comment
There was a problem hiding this comment.
Sorry to request changes but it'd be great to use XCTAssert(No)Throw instead of do {} catch {} which is error prone (we had a security issue because of it)
| } | ||
| #endif | ||
| return self.eventLoop.makeFailedFuture(error) | ||
| return eventLoop.makeFailedFuture(error) |
There was a problem hiding this comment.
think this is the wrong way around, no?
There was a problem hiding this comment.
No, I think this is correct, all channel related stuff should be on channel EL, not on providers default EL
There was a problem hiding this comment.
@artemredkin sorry, I meant from self. to missing self, eventLoop isn't a local variable, is it?
There was a problem hiding this comment.
it is, yes, this is a channel EL, I can rename it to make it more obvious
There was a problem hiding this comment.
@artemredkin yes, I think that'd be a good idea tbh
| } | ||
| #endif | ||
| return self.eventLoop.makeFailedFuture(error) | ||
| return eventLoop.makeFailedFuture(error) |
There was a problem hiding this comment.
@artemredkin sorry, I meant from self. to missing self, eventLoop isn't a local variable, is it?
3 participants
Notify delegates about connect errors
Motivation:
When channel is established, all errors from that channel are piped into
didReceiveErrormethod of theHTTPClientReponseDelegate, but connection errors are only delivered to task's promise. That behaviour might be confusing, since there could be cases where task and delegate observe different errors.Modifications:
Send error to delegate in addition to task's promise
Result:
Closes #242