spin icon indicating copy to clipboard operation
spin copied to clipboard

Restructure module stdio

Open lann opened this issue 2 years ago • 3 comments

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

  • #431
  • #438
  • #449
  • #482
  • #513 (includes changes to "follow logs" config)

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 (https://github.com/fermyon/spin/issues/438#issuecomment-1119068691)
  • 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

lann avatar May 23 '22 16:05 lann

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 avatar May 23 '22 19:05 itowlson

We should pick up #514 as part of this.

itowlson avatar May 24 '22 02:05 itowlson

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

itowlson avatar May 24 '22 03:05 itowlson

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

dicej avatar Aug 31 '22 22:08 dicej

This is pretty much subsumed by #763

lann avatar Sep 20 '22 16:09 lann