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

The ZFuture and Runnable traits need re-design #244

Conversation

jerry73204
Copy link
Contributor

This PR aims to reduce potentially blocking code in async context. I review the definition of Runnable and ZFuture traits and find something alarming to me.

  1. The Runnable trait binds a run() method to a type. run() is not async and can potentially block.
  2. The ZFuture adds a wait() to a future type. It blocks on the wrapped future by async_std::task::block_on(self.0).
  3. The derive_zfuture! macro implements ZFuture and Future on a type with Runnable. In the polling function, it calls self.run().

The pitfall stems from step 3. Given that run() can potentially block, it can implicitly blocks the polling function. The developer could fall victim to it because it hides beneath the derive_zfuture! macro.

OTOH, the future.wait() should be used with caution. It could be accidentally called in async context and thus block the poll. The only relevant situations are calling async function in drop() and exported C API. For several reasons, it is encouraged to let calling function to be async rather than the wait() workaround. I would propose these changes.

  • Builder types provides an async fn build() instead of being a future itself. Buidler::new().set_x(x).build().await is more explicit than Buidler::new().set_x(x).await. It also removes the manual impl Future on Builder {}.
  • Remove wait() across the zenoh source code except drop(). Make functions calling wait() to be async.
  • Remove Runnable trait. Move the code within run() to Future::poll() or an async method of that type.
  • Let wait() to be private to public user because it must run in async-std context and user may not respect that.

@Mallets
Copy link
Member

Mallets commented Jun 16, 2022

A deeper discussion is ongoing here: eclipse-zenoh/roadmap#17

@p-avital
Copy link
Contributor

Closing this issue/PR, as the re-design has been done, in favour of the SyncResolve and AsyncResolve traits.

@p-avital p-avital closed this Sep 22, 2022
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.

3 participants