feat: replace Encrypted/Plain clients with foxy library #39
Conversation
Instead, desctruct the object. I'm not sure how to actually shut down the socket cleanly without any race conditions, but it seems to work if we just let the object be destroyed.
This reverts commit c28e803.
|
This pull request has been linked to Shortcut Story #199702: Refactor HTTP operations to use foxy library. |
|
Adding foxy broke gtest; trying to fix. Issue is that its unit tests require Catch2, which I didn't want to pull in. |
| client_builder.write_timeout(http_properties.WriteTimeout()); | ||
|
|
||
| client_builder.connect_timeout(http_properties.ConnectTimeout()); | ||
|
|
There was a problem hiding this comment.
The TODO here is because the SSE stream has a heartbeat, so it's not quite the same as a generic HTTP request read timeout.
For instance, it might be appropriate to have a 10 second read timeout on a poll request or whatever, but that could be too short/long for the heartbeat.
I don't know if the heartbeat interval is actually documented anywhere. Will look.
| return *maybe_val; | ||
| } | ||
| return boost::none; | ||
| } |
There was a problem hiding this comment.
I feel like there's a better way to do this conversion. The gist is foxy takes an boost::optional<context&>, but we're generally using std::optional (which doesn't support references) or std::shared_ptr, in this case.
There was a problem hiding this comment.
I have encountered a similar problem a dozen times. Drives me crazy. With std::optional you have to use the reference_wrapper, which is terrible.
| ->Run(); | ||
| std::string service = | ||
| request->Port().value_or(request->Https() ? "https" : "http"); | ||
|
|
There was a problem hiding this comment.
What's happening here is we're determining whether or not to pass in a ssl_context based on the presence or absence of https as the scheme of the URL.
If you instead pass a port, it's going to pass that in instead - without an ssl_context. So if you did :443, you better use https:// as the scheme if that's a TLS server.
There was a problem hiding this comment.
I don't think we want to support snews:// we just case about maybe some close cases, like domain sockets. So, it would still be fine, you don't need a TLS domain socket.
| port_ = uri_components->port(); | ||
| } else { | ||
| port_ = is_https_ ? "443" : "80"; | ||
| } |
There was a problem hiding this comment.
No longer attempting to set a default port - we can't assume these ports will be correct.
There was a problem hiding this comment.
If it handles this, then that is cool. But the RFCs for HTTP and HTTPs specify the default ports. I guess the only way we could not be sure was if it was not http/https.
There was a problem hiding this comment.
(Technically adding port 21 for FTP would have covered URL cases, but if we are supporting URIs, then that is a different case. Which is probably good to support for people who do strange stuff, even if most of the SDKs don't).
This PR updates the eventsource and requester libraries with
foxy, a slightly higher level networking lib built on asio and beast.The main advantages of using this library over homegrown code are:
I've tried to do the most minimally invasive thing here, which was:
FoxyClientalongside theEncryptedandPlaintextclients in bothAsioRequesterand EventsourceFoxyClientEncryptedandPlaintextclientThis means there is more we could do to take advantage of foxy.
For example, in
AsioRequesterwe could implement retries/redirects in terms of composed asynchronous operations.