-
Notifications
You must be signed in to change notification settings - Fork 36
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
Migrate to use AsyncHTTPClient #15
Conversation
@0xTim This seems to have a merge conflict with your other PR I merged a few days ago |
@alexanderjordanbaker resolved! |
Hey @0xTim as you saw I'm messing around with the errors found in your build, do you have any ideas on those? I didn't get them building locally |
@alexanderjordanbaker SwiftPM issue, think the latest build should fix it |
let response = try await httpClient.execute(urlRequest, timeout: .seconds(30)) | ||
var body = try await response.body.collect(upTo: 1024 * 1024) | ||
guard let data = body.readData(length: body.readableBytes) else { | ||
return .nonTerminalError(OCSPFetchError()) |
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.
Why is this not a terminalError? If we are unable to read data from the OCSP response, wouldn't that be an unrecoverable error?
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.
Err I seem to remember it being caught elsewhere but yeah you're right. I'll update
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.
LGTM, thank you for this work
URLSession is broken, incomplete and generally just a mess on Linux. For server-side applications to recommendation is to use AsyncHTTPClient. This PR migrates from
URLSession
to use AsyncHTTPClient instead.Resolves #4