Skip to content

Conversation

@MungoG
Copy link

@MungoG MungoG commented Oct 2, 2025

No description provided.

double price = 0;

// Design note: the json parsing could be done without using the exception
// interface, but the resulting code would be considerably more verbose.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to fix that then, or explore alternatives

// so that the next async_* function is not called until *after* the previous asynchronous
// function is complete. However the strand is included since it makes our threading assumptions
// explicit for future developers.
boost::asio::strand<boost::asio::io_context::executor_type> strand_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of a strand is not really to queue operations, it is to ensure that two operations do not execute concurrently. A subtle distinction.

"we could "get away" without using a strand," is not quite true, because of composed operations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't understand. What do you mean by "composed operations" in this context?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs an explanation too big for here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdarwin
Copy link
Contributor

sdarwin commented Oct 3, 2025

please rebase on develop, to try ashtum's CI modifications.

template<class Executor>
class historic_fetcher_op
{
enum class state {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of state seems unnecessary, the majority of completion handlers have unique signature and for those that don't (like write and read) we can define a tag type and use asio::prepend to prepend the tag value.
It would look like something like this:

struct on_write{};
struct on_read{};

// signature for write
void operator()(
        Self& self
        on_write,
        , system::error_code ec
        , std::size_t bytes_transferred);

// signature for read
void operator()(
        Self& self
        on_read,
        , system::error_code ec
        , std::size_t bytes_transferred);

// call site 1
beast::http::async_write(
    ssl_stream_,
    request_,
    asio::prepend(std::move(self), on_write{})
);

// call site 2
beast::http::async_read(
    ssl_stream_,
    buffer_,
    response_,
    asio::prepend(std::move(self), on_read{})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting technique! I hadn't thought of this. You can also use the fauxroutines (in boost/asio/coroutine.hpp)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with using fauxroutines here is that we have a few incompatible function call signatures:

// resolve
void operator()(
    Self& self
    , system::error_code ec
    , asio::ip::tcp::resolver::results_type results)

// connect
void operator()(
    Self& self
    , system::error_code ec
    , asio::ip::tcp::resolver::results_type::endpoint_type ep)

// write/read
void operator()(
        Self& self
        , system::error_code ec
        , std::size_t bytes_transferred);

Although technically, we can only use the fauxroutine for the write/read operations, because that's where we have to deal with more than one operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of asio::prepend is that it creates multiple different function template instantiations of essentially the same asio implementation code

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of asio::prepend is that it creates multiple different function template instantiations of essentially the same asio implementation code

If I'm not mistaken, the completion token type is unique for each composed type anyway (it's essentially the Self object type). So, unless we use different tag types for the same operation, like:

async_write(..., asio::prepend(std::move(self), on_write1{}));
async_write(..., asio::prepend(std::move(self), on_write2{}));

there shouldn't be any difference.

Comment on lines 69 to 75
client_type& client_;
resolver_type& resolver_;
ssl_stream_type& ssl_stream_;

buffer_type& buffer_;
request_type& request_;
response_type& response_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible for historic_fetcher_op to just take a reference to historic_fetcher?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be better, yes

} break;
default: {
// This should not happen.
throw std::logic_error("unreachable");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Composed operations should never throw exceptions, because the exception would be thrown from the call site of asio::io_context::run(). They should instead complete with an error (though in this case, it seems to be only for debugging purposes).

request_.version(11);
request_.method(beast::http::verb::get);
request_.set(beast::http::field::host, client.get_host());
request_.set(beast::http::field::user_agent, BOOST_BEAST_VERSION_STRING);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended not to perform preparation work that modifies the state of the io_object (historic_fetcher in this case) in the constructor. This is because we can create deferred operations that are planned to be executed later, for example:

auto op1 = fetcher.async_historic_fetch(asio::deferred);
auto op2 = fetcher.async_historic_fetch(asio::deferred);
// Both of these deferred operations have called the constructor of the composed operation,
// but neither has been scheduled for execution yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(me, taking notes; master class here)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't know that...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashtum I am slightly confused as to how to make the code completely robust here. Let's have a conversation in the morning if that's okay for you.

Copy link
Member

@vinniefalco vinniefalco Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he means to launch the async ops in a separate member function, such as operator()(Self& self) ? Example: https://github.com/vinniefalco/http_io/blob/b24d776d5ac6243099828a161f8f0f9265c1b356/example/client/visit/main.cpp#L112

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the preparation work should happen inside operator()(Self& self) instead of the constructor.

, request_
, response_)
, token
, exec);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of taking exec in the argument we can use the ssl_stream_'s executor:

ssl_stream_.get_executor();

In case of asio::async_compose you can just directly pass the io_object itself (ssl_stream_):

return asio::async_compose<
    CompletionToken,
    void(system::error_code)>(
        historic_fetcher_op<Executor>(
            *this
            , resolver_
            , ssl_stream_
            , buffer_
            , request_
            , response_)
        , token
        , ssl_stream_);

Copy link
Member

@vinniefalco vinniefalco Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does raise the question of how best to parameterize the template types. should it be typename Executor? typename Stream?

} break;
default: {
// This should not happen.
throw std::logic_error("unreachable");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have BOOST_UNREACHABLE for this (spelling?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just what I was looking for, thanks (as std::unreachable is not available yet).

, asio::ip::tcp::resolver::results_type results)
{
if (ec) {
state_ = state::error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean anything if we are completing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. When I put this in I thought I might have to check the state of the object after io.run finished.

However Mohammad's tag trick is neater than having a state (a full state machine is overkill for this and just makes the code harder to read)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should check out fauxroutines

switch (state_) {
case state::resolving: {
// Set a timeout on the operation
beast::get_lowest_layer(ssl_stream_).expires_after(std::chrono::seconds(30));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this depends on the lowest layer being beast::basic_stream or something like that. The stream with the built-in timeout. I would prefer we didn't use that, because it is an overly complex and cumbersome design made obsolete by asio's new "cancelation" features. We don't want to teach it and we should probably deprecate it in beast somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants