-
-
Notifications
You must be signed in to change notification settings - Fork 27
WIP gren run stdin fix #344
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
Conversation
src/Main.gren
Outdated
RunExited exitCode -> | ||
let | ||
_ = | ||
Debug.log "Exited" {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never see this fire if the program I'm running reads from stdin. For example, this program works fine:
main =
Node.defineSimpleProgram <| \env ->
Stream.Log.line env.stdout "Done"
|> Node.endSimpleProgram
gren run Main
will show the Exited
log and exit.
But this one will hang after hitting enter and never show the Exited
log:
main =
Node.defineSimpleProgram <| \env ->
Stream.readBytesAsString env.stdin
|> Task.onError (\_ -> Task.succeed "")
|> Node.endSimpleProgram
But if you run the same program directly (node .gren/app
) the program does not hang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know what's going on.
[ Task.succeed { source = output, target = model.stdout }
, Task.succeed { source = model.stdin, target = input }
, Task.succeed { source = error, target = model.stderr }
]
|> Array.map (Task.attempt RedirectTerminalIO)
|> Cmd.batch
RedirectTerminalIO
will call Stream.read
on model.stdin
twice, once for the compiler, then again for the program to run.
The second time Stream.read
is called, it will fail with a Locked
error (as we're still waiting for the Stream.read
to finish from the compilation. The program will never receive a message on its stdin
, as the Stream.read
has failed.
I think we need to handle stdin
specially.
I would create a RedirectStdin
message, which begins by calling Stream.read model.stdin
. Once/if we get some data from stdin
, we can pass that content on to model.stdinRedirectTarget
(or something) which is a Maybe Stream.Writable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding you, but the program does receive stdin. For example:
main =
Node.defineSimpleProgram <| \env ->
Stream.readBytesAsString env.stdin
|> Task.onError (\_ -> Task.succeed "")
|> Task.andThen (Stream.Log.line env.stdout)
|> Node.endSimpleProgram
If I enter text it will print, but still hang and never show the Exited log.
Regardless, I still think it's worth trying to handle stdin redirection separately. I'll give it a shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if what I want is a connection type for spawn that results in node calling the child process with detached: true
and stdio: inherit
. That seems to be how you'd get the behavior I want: "spawn this with shared stdio but let the parent program exit" and wouldn't require run to do any stdio redirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, actually Integrated seems to be doing what I want. Going to do some more testing and cleanup and get a real PR out ready for review, might take until sometime tomorrow.
The streams that were being passed are never used.
Going to open a new PR for a clean review of the fix |
Working on #341