madmongo1 / blog-new-year-2021

Connection Caching HTTP
Boost Software License 1.0
13 stars 5 forks source link

co_spawn executor #1

Open gummif opened 3 years ago

gummif commented 3 years ago

First, thanks for this interesting blog post.

There is one thing I don't quite understand, about this

Had we written: : co_await net::cospawn(impl->get_executor(), op(), net::use_awaitable); then op would have been called during the setup of the call to cospawn, which would result in impl->call(...) being called on the wrong executor/thread.

So if we have e.g.

awaitable<void> some_coro();

awaitable<void> other()
{
  co_await co_spawn(ex, some_coro(), net::use_awaitable);  // wrong?
  co_await co_spawn(ex, []() -> awaitable<void> { co_await some_coro(); }, net::use_awaitable); // OK?
  co_await co_spawn(ex, []() -> awaitable<void> { return some_coro(); }, net::use_awaitable); // OK?
}

is the first case not called on the correct executor (or maybe just initialized on the wrong one?), but the other two are fine?

madmongo1 commented 3 years ago

You know what? I think I’m wrong in the blog.

Asio coroutines are lazy. That is, they always suspend until the use of co_await.

So while the coroutine frame of op() would have been constructed on the callers executor, the actual co_await and therefore execution of the coroutine wold happen on the inner executor as expected.

What I am really guarding against here is losing any variables captured by op.

Even this is unnecessary since we are co_awaiting the spawned coroutine, which means that the caller’s coroutine frame and everything in it will still exist during the forward progress of op().

So the entire comment is just a result of my paranoia. :)

In this case, either op or op() would be fine.

This would not be the case if, for example, op was spawned as detached. In that case we’d want to capture it.

Perhaps a better solution would be for op() to call a static coroutine which takes the impl as a by-value argument. Then there’s no ambiguity and it works in all cases.

I like this approach as the coroutine now becomes the actor and the impl is just some shared state.

May I copy your question into an additional section in the blog and quote you?

R

On Fri, 26 Feb 2021 at 16:06, Gudmundur Adalsteinsson < notifications@github.com> wrote:

First, thanks for this interesting blog post.

There is one thing I don't quite understand, about this

Had we written: : co_await net::cospawn(impl->get_executor(), op(), net::use_awaitable); then op would have been called during the setup of the call to cospawn, which would result in impl->call(...) being called on the wrong executor/thread.

So if we have e.g.

awaitable some_coro();

awaitable other() { co_await co_spawn(ex, some_coro(), net::use_awaitable); // wrong? co_await co_spawn(ex, []() -> awaitable { co_await some_coro(); }, net::use_awaitable); // OK? co_await co_spawn(ex, []() -> awaitable { return some_coro(); }, net::use_awaitable); // OK? }

is the first case not called on the correct executor (or maybe just initialized on the wrong one?), but the other two are fine?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/madmongo1/blog-new-year-2021/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHOZSMHCSU2M42EXDHYWCDTA62IVANCNFSM4YITBHUQ .

-- Richard Hodges hodges.r@gmail.com office: +442032898513 home: +376841522 mobile: +376380212

gummif commented 3 years ago

Thanks for the reply, that makes sense (otherwise co_spawn would be quite dangerous).

You can quote me.

gummif commented 3 years ago

One more thing related to this if you have time, I was implementing a generic utility function for fun to do this. I get a segfault (and asan stack-buffer-overflow) if I implement the following without assigning the current executor to a variable.

    template<class Executor, class F>
    [[nodiscard]] auto co_dispatch(const Executor& ex, F&& f) -> asio::awaitable<typename std::invoke_result_t<F&&>::value_type>
    {
        auto cur = co_await asio::this_coro::executor;
        if (ex == cur)
// crash with: if (ex == co_await asio::this_coro::executor)
        {
            co_return co_await std::forward<F>(f)();
        }
        else
        {
            co_return co_await asio::co_spawn(ex, std::forward<F>(f), asio::use_awaitable);
        }
    }

used like so

        [[nodiscard]] asio::awaitable<void> async_acquire()
        {
            co_await co_dispatch(m_strand, [this]() -> asio::awaitable<void> {
                // crash before reaching this line
                assert(m_counter >= 0);
                if (m_counter == 0)
                {
                    asio::error_code ec;
                    co_await m_timer.async_wait(asio::redirect_error(asio::use_awaitable, ec));
                    assert(m_counter > 0);
                }
                --m_counter;
            });
        }

Any ideas what could cause this?

edit: taking ex by value does not crash which is interesting.

madmongo1 commented 3 years ago

I've noticed some instability with gcc10. I currently limit coroutine compilations to clang-11, which seems more reliable at present.

Otherwise, it should be fine.

On Sat, 27 Feb 2021 at 17:22, Gudmundur Adalsteinsson < notifications@github.com> wrote:

One more thing related to this if you have time, I was implementing a generic utility function for fun to do this. I get a segfault (and asan stack-buffer-overflow) if I implement the following without assigning the current executor to a variable.

template<class Executor, class F>
[[nodiscard]] auto co_dispatch(const Executor& ex, F&& f) -> asio::awaitable<typename std::invoke_result_t<F&&>::value_type>
{
    auto cur = co_await asio::this_coro::executor;
    if (ex == cur)// crash with: if (ex == co_await asio::this_coro::executor)
    {
        co_return co_await std::forward<F>(f)();
    }
    else
    {
        co_return co_await asio::co_spawn(ex, std::forward<F>(f), asio::use_awaitable);
    }
}

used like so

    [[nodiscard]] asio::awaitable<void> async_acquire()
    {
        co_await co_dispatch(m_strand, [this]() -> asio::awaitable<void> {
            // crash before reaching this line
            assert(m_counter >= 0);
            if (m_counter == 0)
            {
                asio::error_code ec;
                co_await m_timer.async_wait(asio::redirect_error(asio::use_awaitable, ec));
                assert(m_counter > 0);
            }
            --m_counter;
        });
    }

Any ideas what could cause this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/madmongo1/blog-new-year-2021/issues/1#issuecomment-787097488, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHOZSMRI4ROXAOC7LD3PZ3TBEL3BANCNFSM4YITBHUQ .

-- Richard Hodges hodges.r@gmail.com office: +442032898513 home: +376841522 mobile: +376380212

gummif commented 3 years ago

An update on this issue. Turns out coroutines make it really easy to use dangling references (the lambda is destroyed before being executed). The solution is to capture the function arguments by value:

template<class Executor, class F>
[[nodiscard]] auto co_dispatch(Executor ex, F f)
{
...
}
madmongo1 commented 3 years ago

An update on this issue. Turns out coroutines make it really easy to use dangling references (the lambda is destroyed before being executed). The solution is to capture the function arguments by value:

With very few exceptions, the only thing you should do with an awaitable is await it. In this way the references won't dangle as they will be valid for the duration of the coroutine. But yes, passing by value is always correct.

gummif commented 3 years ago

I see. I have been experimenting, and both clang and gcc trunk agree, but gcc 10.2 destructs temporary arguments even if the coroutine/awaitable is immediately awaited (see trunk https://godbolt.org/z/6nzj5TMc9 vs 10.2 https://godbolt.org/z/nTvEz9dh4).