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

How do you run final cleanup with a chain? #58

Open
geiseri opened this issue Nov 9, 2022 · 8 comments
Open

How do you run final cleanup with a chain? #58

geiseri opened this issue Nov 9, 2022 · 8 comments
Labels

Comments

@geiseri
Copy link

geiseri commented Nov 9, 2022

@Naios

I am currently using https://github.com/xhawk18/promise-cpp and would like to move to continuable since I am using native asio. One thing that is holding me back is the Promise::finally(FUNC_ON_FINALLY on_finally) method. I use this because I need to have a few cleanup operations that can only run after the entire chain has been resolved. I was having issues finding a way to make this with next(...) without duplicating code in both functions. Am I missing something?

@Naios
Copy link
Owner

Naios commented Nov 9, 2022

@geiseri Could you maybe provide a code example what you have tried so far?

@geiseri
Copy link
Author

geiseri commented Nov 12, 2022

This is the original code: My issue is that this chain resolves at a future time and I need inst to live on.

promise::Promise  getResponse(Type symbolType, std::string symbol) {
  auto inst = registry.get(symbolType); // This is a shared ptr 
  return inst->resolveScore(symbol).finally([inst ]() {(void)inst;});
}

I got this far, but it crashes with inst going away somewhere along the way.

cti::continuable<StrategyResponse> getResponse(Type symbolType, std::string symbol) {
  auto inst = registry.get(symbolType); // This is a shared ptr 
  return inst->resolveScore(symbol)
      .then([inst](StrategyResponse res) {
          (void)inst;
          return cti::make_ready_continuable(res);
      });
}

If I pull the inst out of the function and pass it in, it works just fine, but the goal of the method in the first place was to hide the registry.get(...) part from the public API. I think the issue might be that the finally runs after the whole promise chain is resolved, where then will release it right away.

Does that make more sense?

@Spongman
Copy link

could it be that you're allowing the cti::continuable returned from getResponse to be destroyed prematurely? how are you calling getResponse ?

@geiseri
Copy link
Author

geiseri commented Nov 15, 2022

It is when I am still in a .then(...) call. Is that a different instance of the continuable? Or is that different? I was under the impression that inst would stay in scope until the resolution of the final handler in the chain. That being said, it could be that there is another issue going on that was hidden by the promise::Promise implementation.

@Spongman
Copy link

Spongman commented Nov 16, 2022

It is when I am still in a .then(...) call. Is that a different instance of the continuable?

Not quite sure what you mean by this. It might be helpful to include more context to how you’re calling this function, and how you’re handling the lifetime of the continuable that it returns. There’s nothing wrong with the code you’ve shown here, so the error must be elsewhere.

Maybe put a breakpoint in your ‘inst’ object’s destructor and look at the stack trace.

A couple of other things:

  • if your resolveScore function throws, then you'll need to handle the exceptions in a .fail clause (in addition to the .then clause)
  • the return cti::make_ready_continuable(res); is unnecessary, you can just return res; from a .then callback.
  • you may be able to avoid doing this entirely if you just make your inst object derive from enable_shared_from_this, and make sure that whatever inner async operations are performed within resolveScore hold onto a shared_ptr reference to this.

@Naios
Copy link
Owner

Naios commented Nov 16, 2022

I second what @Spongman says. Tracing the destructor of your shared_ptr object through a debugger is probably your best option.

Additionally I have to add that the callable that you pass to then, fail, or next stays valid until it can be potentially called, but not post that. Callables are destructed in generally immediatly when they are no longer needed for the chain.

So if you require your handle/shared_ptr to stay alive afterwards you need to reference it in the handler you are using the handle in.

Btw. you can also use next as an "catch all" handler which could be used to implement something like finally.

@Naios Naios added the question label Nov 16, 2022
@geiseri
Copy link
Author

geiseri commented Dec 5, 2022

Okay, I think I found the issue. Originally the code used .finally inside of the method that returned the continuation. It looks like all .finally handlers were run after the entire chain was resolved. It seems like I need to move that shared pointer creation into the caller, and pass it into the method. I was wondering though. I am using asio there is an ability to add values that are passed to the completion handler via the executor. I did not see an obvious method to convert back and forth from a continuable back to the underlying asio executor. I assume that is not easy since a continuable may or may not have initially been created from there. I can always return a tuple with both data values though.

@Naios
Copy link
Owner

Naios commented Dec 5, 2022

There is no mechanism to allow a true finally handler that runs when the entire chain has ended because in the process of building the chain you might always add further continuations.

The executor is getting passed down into the continuation and therefore is not accessible anymore from outside.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants