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

feat(rust): provide async implementations of blocking entity and vault functions #1832

Closed
wants to merge 9 commits into from

Conversation

antoinevg
Copy link
Contributor

Proposed Changes

There's a high risk of blocking the executor when scheduling blocking functions in an environment without pre-emptive multitasking.

This branch is a preliminary exploration of the changes required to drop the use of blocking calls in the Ockam code base.

Specifically, this branch factors out the usage of ockam_node::block_future aka tokio::task::block_in_place

@antoinevg antoinevg marked this pull request as draft September 6, 2021 15:20
@antoinevg antoinevg force-pushed the antoinevg/nonblocking branch 6 times, most recently from 0569f85 to cdf5b78 Compare September 7, 2021 16:37
@thomcc
Copy link
Contributor

thomcc commented Sep 9, 2021

I'd like to see how badly my workspace stuff mucks things up here, have you pushed /Users/antoine/Ockam/ockam_executor up anywhere?

@antoinevg
Copy link
Contributor Author

I'd like to see how badly my workspace stuff mucks things up here, have you pushed /Users/antoine/Ockam/ockam_executor up anywhere?

I've added you to the repo. The code is really filthy and may or may not compile under std right now so please excuse the mess.

You may also want to start off first with the antoinevg/nostd+alloc branch before tackling this one, it's had most of my attention this week and I haven't rebased the changes onto here yet. It also references the git repos rather than local paths.

@antoinevg antoinevg linked an issue Sep 9, 2021 that may be closed by this pull request
@antoinevg antoinevg force-pushed the antoinevg/nonblocking branch 15 times, most recently from 71d6972 to 7a6d7f4 Compare September 29, 2021 14:05
@antoinevg
Copy link
Contributor Author

antoinevg commented Sep 30, 2021

Some outstanding issues needed to land this PR

AsyncClone trait

Code Duplication

  • Most of the new async methods are simple duplications of the sync method.
    • e.g. in ockam_entity/src/authentication.rs :: generate_proof() / async_generate_proof()
  • Two thoughts on approaches:
    1. Make the async method contain the implementation and just call it from the sync method via block_future?
    2. Write a macro that will automagically provide the sync implementation?

Incomplete API coverage

  • Can identify missing methods by searching for methods that use block_future and that lack a corresponding async_ prefixed implementation.
    • e.g. TcpRouterHandle :: clone(), CredentialProtocol :: create_credential_issuance_listener etc.
  • Some methods no longer have a sync implementation because their parents were originally async - is this okay?
    • e.g. in ockam_entity/src/worker/create_key.rs the pub fn create_key_static() method has been replaced by pub async fn async_create_key_static() because it is called by ockam_entity/src/profile_state.rs :: pub(crate) async fn async_create() which has always been an async function.

General questions

  • Are we okay with standardizing on an async_ prefix in front of method names.
  • What happens with the pre-existing methods that are async but are not prefixed?
    • e.g. ockam_channel/src/secure_channel.rs :: pub async fn create()
  • Should the async_* methods live in the same trait as the sync methods or should we move them to their own Async* prefixed trait?

Ugly Hacks

  • This whole exercise started as a proof of concept so code layout is pretty ugly:
    • Methods without a line-space between them.
    • Missing docs.
    • e.g. ockam_entity/src/channel/trust_policy/all_trust_policy.rs
  • Switching methods to async often required tighter trait bounds on associated structs.
    • More than one place where the trait bounds are specified in the implementation where they could simply have been aliased in the trait definition. e.g. ockam_entity/src/channel/listener.rs

@antoinevg antoinevg force-pushed the antoinevg/nonblocking branch 10 times, most recently from 094fabb to 87aebbc Compare October 11, 2021 11:44
@SanjoDeundiak
Copy link
Member

Can be closed I guess? @antoinevg

@antoinevg antoinevg closed this Oct 28, 2021
@antoinevg antoinevg deleted the antoinevg/nonblocking branch November 16, 2021 16:19
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.

Ockam Node for Embedded no_std enironments
3 participants