Skip to content
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

support lazy callback #376

Closed
wants to merge 2 commits into from
Closed

Conversation

qicosmos
Copy link
Collaborator

@qicosmos qicosmos commented Mar 19, 2024

Why

allow lazy start and collectAny callback support lazy.

motivation

Use a kotlin example to show the motiviation:

private suspend fun runFollower() {
        val timer = ElectionTimer(2500)
        timer.schedule()

        while (state == RaftState.FOLLOWER) {
            select<Unit> {
                timer.channel.onReceive {
                    state = RaftState.CANDIDATE
                }
                commands.onReceive { command ->
                    when (command) {
                        is RequestVoteCommand -> {
                            if (command.term >= currentTerm) {
                                commandResults.send(RequestVoteCommandResult(currentTerm, true)) //#1 call another coroutine
                            } else {
                                commandResults.send(RequestVoteCommandResult(currentTerm, false)) //#2 call another coroutine
                            }
                        }
                        is AppendEntriesCommand -> {
                            timer.reset()
                        }
                    }
                }
            }
        }
    }

The application need to call another coroutine in the select callback. Current collectAny callback don't support Lazy, can't call another coroutine, so need to support the feature.

What is changing

Example

    auto test0 = []() mutable -> Lazy<int> { co_return 41; };

    auto test1 = []() mutable -> Lazy<int> { co_return 42; };

    test1().start([&](auto val) -> Lazy<void> {
        int r = co_await test0();
        int result = r + val.value();
        EXPECT_EQ(result, 83);
        co_return;
    });

@ChuanqiXu9
Copy link
Collaborator

Please describe the motivation, the design and the use cases more clearly. Also we need a document this.

@qicosmos
Copy link
Collaborator Author

Please describe the motivation, the design and the use cases more clearly. Also we need a document this.

has added motivation.

Copy link
Collaborator

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

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

Sorry, I feel the doc may not clear.

Also if I understand correctly, this may not be something we want. This has the semantics as then semantics. And we should co_await directly to get the value and write the logics directly instead chaining the callbacks.

This is a regression from then-style futures to coroutine tasks.

@qicosmos
Copy link
Collaborator Author

Sorry, I feel the doc may not clear.

Also if I understand correctly, this may not be something we want. This has the semantics as then semantics. And we should co_await directly to get the value and write the logics directly instead chaining the callbacks.

This is a regression from then-style futures to coroutine tasks.

It's not relate with then, only allow co_await in callback, as i showed example, we need co_await another coroutine in callback.

@ChuanqiXu9
Copy link
Collaborator

I think this may be over designed.

On the detail side, it violates our principles to start new chain of Lazies without binding executors.

On the higher level side, if we want to implement semantics "do something after another gets ready", we should use co_await and makes this to be a coroutine and calls that.

@qicosmos
Copy link
Collaborator Author

qicosmos commented Mar 20, 2024

I think this may be over designed.

On the detail side, it violates our principles to start new chain of Lazies without binding executors.

On the higher level side, if we want to implement semantics "do something after another gets ready", we should use co_await and makes this to be a coroutine and calls that.

So, how to implement the showed example with current collectAny? why don't support lazy callback, lazy is also a function, of course canbe a callback, lazy as callback is common, not over designed.

std::remove_cvref_t<decltype(*callback)>, size_t,
std::remove_cvref_t<decltype(result)>>;
if constexpr (HasMemberCoAwaitOperator<U>) {
(*callback)(i, std::move(result)).start([](auto&&) {
Copy link
Contributor

@poor-circle poor-circle Mar 20, 2024

Choose a reason for hiding this comment

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

On the detail side, it violates our principles to start new chain of Lazies without binding executors.

@ChuanqiXu9 @qicosmos

We could binding executores here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It may not be correct since we can change the executor of the current lazy during its execution. See dispatch.

@poor-circle
Copy link
Contributor

poor-circle commented Mar 20, 2024

On the higher level side, if we want to implement semantics "do something after another gets ready", we should use co_await and makes this to be a coroutine and calls that.

@ChuanqiXu9 @qicosmos

assume we have 4 lazy task: A, B, C ,D. B depends on A and D depends on C. Now we want to start those works.

For this PR:

A().start([](auto&&)->Lazy<void> { co_await B();});
C().start([](auto&&)->Lazy<void> { co_await D();});

The old solution is:

 []()->Lazy<void>{
    co_await A();
    co_await B();
 }().start();
 []()->Lazy<void>{
    co_await C();
    co_await D();
 }().start();

@qicosmos
Copy link
Collaborator Author

Or just let collectAny callback support Lazy, don't modify Lazy start?

@ChuanqiXu9
Copy link
Collaborator

As the example showed by @poor-circle (except we still need to set the executor), it is not complicated to do it now.

why don't support lazy callback, lazy is also a function, of course canbe a callback,

No. There are million cases that the rules applied to functions can't be applied to coroutines.

lazy as callback is common, not over designed.

I never see any design that a callback can accept a lazy-like task. It is not common for sure. Also one of the main motivations of lazy-like coroutines is to avoid callback(hell)s. It is a dangerous design direction to try to make the callback mechanism more complicated.

If we need to deal with the result returned first from collectAny, this is the job of co_await collectAny.

@qicosmos
Copy link
Collaborator Author

qicosmos commented Mar 21, 2024

As the example showed by @poor-circle (except we still need to set the executor), it is not complicated to do it now.

why don't support lazy callback, lazy is also a function, of course canbe a callback,

No. There are million cases that the rules applied to functions can't be applied to coroutines.

lazy as callback is common, not over designed.

I never see any design that a callback can accept a lazy-like task. It is not common for sure. Also one of the main motivations of lazy-like coroutines is to avoid callback(hell)s. It is a dangerous design direction to try to make the callback mechanism more complicated.

If we need to deal with the result returned first from collectAny, this is the job of co_await collectAny.

https://godbolt.org/z/rcWhsfGMx
here is a go select example, can call a new go coroutine in select callback, it's common in go and kotlin, i think c++ also need do this in real applications.

Lazy coroutine can help to reduce callback hell, but can't stop user write callback hell code with Lazy, it's up to users, current Lazy still can write callback hell code:

async_simple::coro::Lazy<void> f1(){
  co_return;
}

async_simple::coro::Lazy<void> f2(){
  co_return;
}

async_simple::coro::Lazy<void> f3(){
  co_return;
}

async_simple::coro::Lazy<void> f4(){
  co_return;
}

int main() {
  auto foo = []()->async_simple::coro::Lazy<void> {
    f1().start([](auto&&){
      f2().start([](auto&&){
        f3().start([](auto&&){
          f4().start([](auto&&){});
        });
      });
    });
    co_return;
  };

  async_simple::coro::syncAwait(foo());
}

so i think callback function can't be a Lazy function is not reasonable, we should not do restrict here, let the user choose
the callback function type is better.

@ChuanqiXu9
Copy link
Collaborator

https://godbolt.org/z/rcWhsfGMx
here is a go select example, can call a new go coroutine in select callback, it's common in go and kotlin, i think c++ also need do this in real applications.

Coroutines in different languages are different things. e.g., the go coroutine is actually a stackful coroutine. And calls a coroutine in other languages may mean to send the coroutines to the executor. In async_simple, it corresponds to Lazy::start, not co_await. Also in async_simple, the executor doesn't know anything about coroutines by design.

Then I don't think we can mimic select in Go well with collectAny in async_simple. They are not symmetric.

but can't stop user write callback hell code with Lazy

No mechanism can stop user writing bad codes. We shouldn't encourage them.

@qicosmos
Copy link
Collaborator Author

qicosmos commented Mar 21, 2024

https://godbolt.org/z/rcWhsfGMx
here is a go select example, can call a new go coroutine in select callback, it's common in go and kotlin, i think c++ also need do this in real applications.

Coroutines in different languages are different things. e.g., the go coroutine is actually a stackful coroutine. And calls a coroutine in other languages may mean to send the coroutines to the executor. In async_simple, it corresponds to Lazy::start, not co_await. Also in async_simple, the executor doesn't know anything about coroutines by design.

Then I don't think we can mimic select in Go well with collectAny in async_simple. They are not symmetric.

but can't stop user write callback hell code with Lazy

No mechanism can stop user writing bad codes. We shouldn't encourage them.

I don't think the usage is related with languages, it's from application requirement.

If we only can do this in statckful coroutine, we need another statckful coroutine library until C++26?

About executor it's not a big problem, one solution: pass current executor to lazy callback; another way: let the lazy callback execute immediately with start, the user will use their executer in the callback, and the callback Lazy function give a chance to use co_await in callback function for user.

@ChuanqiXu9
Copy link
Collaborator

If we only can do this in statckful coroutine, we need another statckful coroutine library until C++26?

It is not about stackful or stackless, it is about we don't have a global by default executor.

And the design for Lazy::start is an entry point for a chain execution and we can record the result of the execution simply. If the user wish to do more complicated job after a lazy task gets done. Please co_await it and start the wrapped lazy. If we want to do something about the task returned first from collectAny, please co_await collectAny and handle the results.

I do think it is bad to abusing callbacks with coroutines.

@qicosmos
Copy link
Collaborator Author

If we only can do this in statckful coroutine, we need another statckful coroutine library until C++26?

It is not about stackful or stackless, it is about we don't have a global by default executor.

And the design for Lazy::start is an entry point for a chain execution and we can record the result of the execution simply. If the user wish to do more complicated job after a lazy task gets done. Please co_await it and start the wrapped lazy. If we want to do something about the task returned first from collectAny, please co_await collectAny and handle the results.

I do think it is bad to abusing callbacks with coroutines.

Ok, got it.
I will finish the select with Lazy callback function in yalantinglibs, this PR will close.

@qicosmos qicosmos closed this Mar 21, 2024
@microcai
Copy link

microcai commented Oct 8, 2024

都是 chinglish 啊,看着脑瓜子疼

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.

4 participants