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

Streaming bin script output in non-typescript kernels #3782

Closed
1 of 2 tasks
jmalins opened this issue Sep 30, 2022 · 4 comments · Fixed by #3794
Closed
1 of 2 tasks

Streaming bin script output in non-typescript kernels #3782

jmalins opened this issue Sep 30, 2022 · 4 comments · Fixed by #3794
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.

Comments

@jmalins
Copy link
Contributor

jmalins commented Sep 30, 2022

Describe the feature

Pipe the output streams of bin scripts invoked from the JSII kernel. Currently, these calls are made using the synchronous InvokeScriptRequest / InvokeScriptResponse synchronous API. The output is buffered until the call completes. This experience is not ideal for longer running scripts with progressive updates.

Use Case

Long-running scripts with progressive updates, such as projen deploy called under Python via the JSII kernel, rather than a globally installed installed projen instance.

These calls were recently made to work via a few updates:

The experience, however, is not ideal, due to the buffered output.

Proposed Solution

Maintain the current synchronous (InvokeScriptRequest / InvokeScriptResponse) API for backwards compatibility, but add a new streaming API that language runtimes can opt into if they are on a platform that supports it. The opt-in approach will allow this to be implemented progressively.

I suspect this will require out-of-band IPC (local domain sockets or named pipes) so that it doesn't disrupt the standard kernel connection streams.

I am not an IPC expert, but one (naive) approach on POSIX OSs would be:

  1. language runtime dynamically creates local sockets to receive the target process stdout and stderr
  2. sockets/FDs identifiers are passed to kernel via a new InvokeScriptWithStreams call
  3. kernel opens streams to the argument file descriptors, passes them to spawnSync of the target process
  4. language runtime receives data on sockets, streams to stdout / stderr on the user process
  5. kernel process completes, returns exit code, probably using existing InvokeScriptResponse structure

I have a nervous suspicion it is more complicated than this (termcap?), and I wouldn't know where to start with Windows. I definitely welcome thoughts on the approach.

Other Information

My team is very interested in building out a first-rate Python dev-tool experience for JSII based libraries, specifically CDK on projen.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.x

Environment details (OS name and version, etc.)

MacOS, Linux

@jmalins jmalins added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Sep 30, 2022
@RomainMuller
Copy link
Contributor

RomainMuller commented Oct 3, 2022

I wonder if we should reverse control here, and instead of having the kernel spawn the child process on behalf of the caller, it merely returned the command, arguments, and possibly environment variables that need to be passed to spawn or equivalent in the local language?

As you're guessing, the IPC situation in Windows versus POSIX platforms is quite complicated, and it turns out to be relatively easy to introduce subtle bugs when attempting to do this (even node fails to completely "hide" the behavioral differences between Windows and the other OS'es, and it might be easier to have something clean if we reduce the amount of intermediates between the ultimate child process and the end-user application).

@jmalins
Copy link
Contributor Author

jmalins commented Oct 3, 2022

@RomainMuller that is a profoundly better approach! 👍

How about this for an API, reusing the InvokeScriptRequest struct for the argument:

// api.ts
export interface GetScriptCommandResponse {
  command: string;
  args: string[];
  env: Record<string, string>;
}

// kernel.ts
export class Kernel {
  // ...
  public getBinScriptCommand(req: api.InvokeScriptRequest): api.GetScriptCommandResponse;
  // ...
}

I presume we would retain the existing invoke API for compatibility purposes. It can be refactored to use the new getBinScriptCommand method internally.

@jmalins
Copy link
Contributor Author

jmalins commented Oct 6, 2022

I realized after getting into the code, that JSII requires a unique argument type for each API call. PR #3794 uses the basic approach above, but with a GetScriptCommandRequest argument that mirrors the parameters of the InvokeScriptRequest

@mergify mergify bot closed this as completed in #3794 Nov 23, 2022
mergify bot pushed a commit that referenced this issue Nov 23, 2022
Resolves #3782.

Inverts control on bin script execution by returning the script to execute (as well as arguments and ENV variables), rather than running it within the kernel process. This allows the calling process to pipe the output streams, rather than the kernel buffering the output and returning it at the end of the synchronous call.

Also, adds more complete tests to the python runtime to validate `stderr`, exit codes and streaming response.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants