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

Restructure module stdio #520

Closed
lann opened this issue May 23, 2022 · 5 comments
Closed

Restructure module stdio #520

lann opened this issue May 23, 2022 · 5 comments
Assignees

Comments

@lann
Copy link
Collaborator

lann commented May 23, 2022

There have been a few recent issues and PRs around the ways stdio is managed for WASI modules:

The current state of the code is somewhat confusing, requiring coordination between the configuration passed in ExecutionContextConfiguration and the trigger executor implementation. Also, the options for stdio handling are limited to capturing to memory, capturing to files, and a follow flag that only works with the "capturing to memory" path.

There a few interrelated requirements to detangle here:

  • Some trigger executors need mandatory control over some of stdio (e.g. stdin/stdout for WAGI)
  • Some spin embedders want to intercept stdio (stdio in spin #438 (comment))
  • Where stdout/err aren't processed by a trigger executor or intercepted by an embedding, we (usually?) want the default behavior to log them to files and/or the console

I think we can restructure to simplify the code while maintaining these features:

  • Collapse the ExecutionContextConfiguration IO-related fields into a single io field typed like:

    struct IoDefaults {
      stdout: IoOutputBehavior,
      stderr: IoOutputBehavior,
    }
    enum IoOutputBehavior {
      Null,
      Log,
      Pipe(WritePipe),
    }
  • Update ExecutionContext::prepare_component's io arg to take a type something like:

    struct IoOverrides {
      stdin: Option<ReadPipe>,
      stdout: Option<WritePipe>,
      stderr: Option<WritePipe>,
    }
  • Update existing redirect_to_mem_buffer usage to use WritePipe::new_in_memory

@itowlson
Copy link
Contributor

There is a minor subtlety where a component can be writing to stdout/stderr as well as to a buffer for logging. We represent this as a Write implementation so it's not hard but it does need to be represented somehow.

The way I envisaged this working was similar to you: it has to be driven from the outside in, with an opportunity for executor code to override. My original thought was that the host would actually compose what you call the IoOverrides - typically using helper methods, but it could be completely custom - and pass that into the trigger (the trigger being a good "unit of hosting"). Using an enum is probably more intention-revealing, though, and thus will likely work more clearly with any future non-monolithic architecture.

In any case, I completely agree about simplifying and streamlining the whole area!

@itowlson
Copy link
Contributor

We should pick up #514 as part of this.

@itowlson
Copy link
Contributor

itowlson commented May 24, 2022

I'm also wondering if we can store the redirect as part of the component object. This puts it in a natural location as part of the component config, which already flows through to the executor, and allows components to be set up differently without the executor having to check a "follow' setting. Components are kept in the manifest object, though, and the loader currently tends to avoid mixing command line settings into that. Might give it an explore though. Gah, no it doesn't. Only the component ID flows through to the executor.

ETA: okay the component objects are in the execution context so we can still get at them

@lann lann self-assigned this May 24, 2022
This was referenced May 25, 2022
@kate-goldenring kate-goldenring moved this to 🆕 Triage Needed in Spin Triage Aug 16, 2022
@dicej dicej moved this from 🆕 Triage Needed to 🏗 In progress in Spin Triage Aug 31, 2022
@dicej
Copy link
Contributor

dicej commented Aug 31, 2022

@lann tells me he's working on a subset of this as part of a more general refactoring, FYI.

@lann
Copy link
Collaborator Author

lann commented Sep 20, 2022

This is pretty much subsumed by #763

@lann lann closed this as completed Sep 20, 2022
Repository owner moved this from 🏗 In progress to ✅ Done in Spin Triage Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants