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

Implement 'enclaver run' #103

Merged

Conversation

russellhaering
Copy link
Contributor

@russellhaering russellhaering commented Oct 31, 2022

Screen.Recording.2022-10-31.at.2.37.14.PM.mov

I created #104 to reflect some future improvements I'd like to make.

Copy link
Contributor

@eyakubovich eyakubovich left a comment

Choose a reason for hiding this comment

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

Some stylistic changes to think about.

}

pub async fn run_enclaver_image(
&mut self,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still finding the best patterns myself but for these structs that have a state machine to them, but it might be better to consume self and then return a new object. For example:

impl RunWrapper {
    pub fn new() -> Result<Self> {
        ...
    }

    pub async fn run_enclave_image(self, ...) -> Result<RunHandle> {
         ...
    }
}

struct RunHandle {
}

impl RunHandle {
    pub async cleanup(self) {
       ...
    }
}

This should remove the Option fields in your structs since they model "not for this state".

Alternatively you can take in a CancellationToken so you end up with:

impl RunWrapper {
    pub fn new() -> Result<Self> {
        ...
    }

    pub async fn run_enclave_image(self, token: CancellationToken) -> Result<()> {
         ...
    }
}

and no cleanup to forget. However you have to make sure you .await to completion.

let shutdown_signal = enclaver::utils::register_shutdown_signal_handler().await?;

tokio::select! {
res = runner.run_enclaver_image(&image_name, port_forwards) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If shutdown_signal comes,the run_enclaver_image will get aborted at some inner .await. However you don't know which one so cleanup now has to account for every case.

@russellhaering russellhaering merged commit fb60797 into edgebitio:main Nov 1, 2022
@russellhaering russellhaering mentioned this pull request Nov 1, 2022
3 tasks
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.

2 participants