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

[ new ] Add support for bi-directional pipes on POSIX systems (resolves #2935) #2944

Merged
merged 3 commits into from
Apr 15, 2023

Conversation

dunhamsteve
Copy link
Contributor

Description

Adds a popen2 function to System.File.Process which allows Idris to run subprocesses with access to both the input and output file streams. It currently doesn't support windows, but updates are welcome if it's possible and somebody knows how to do it (and can verify that it works). This feature was requested by @jfdm in #2935, and I think @ohad was asking about it last year.

With this change you can do:

import System.File

main : IO ()
main = do
    Right (inFile, outFile) <- popen2 "idris2"
        | Left err => printLn err
    _ <- fPutStrLn outFile ":h\n"
    closeFile outFile
    Right result <- fRead inFile
        | Left err => printLn err
    putStrLn result
    putStrLn "done"

But I'd recommend writing be done on a separate thread from reading to avoid deadlocking if the subprocess is waiting for a reader while we're waiting to write.

Let me know if any changes are needed.

Should this change go in the CHANGELOG?

Yes, I added a note to the CHANGELOG.

@jfdm
Copy link
Collaborator

jfdm commented Apr 12, 2023

Thanks for this!

It was on my TODO list for this week, so thanks for checking this item off.

@jfdm
Copy link
Collaborator

jfdm commented Apr 12, 2023

I have had a quick look at the code, it was more easier than I imagined. Kudos.

I do have a minor quibble.

I wonder if a small layer of encapsulation should be added.
That is, one could:

  1. add a record as the 'Right' return type for popen2, thus enabling differentiation between the two handles; and optionally;
  2. extend the API for popen to support working with the returned record.

Then again that could be a bit OTT for such a low-level process API.

@gallais
Copy link
Member

gallais commented Apr 12, 2023

👍 I think it's a good idea to break the symmetry in the type.

The example was confusing to me as I understood in/out to be from the POV of the
callee but it was in fact from the POV of the caller.

Copy link
Collaborator

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

I'm still reading over the C code but I do think we should add a test that touches this new code prior to merging as well.

libs/base/System/File/Process.idr Outdated Show resolved Hide resolved
support/c/idris_file.c Show resolved Hide resolved
support/c/idris_file.c Outdated Show resolved Hide resolved
@dunhamsteve
Copy link
Contributor Author

I was thinking something along the lines of:

||| Result of a popen2 command, containing the 
public export
record SubProcess where
  constructor MkSubProcess
  ||| Process id of the spawned process
  pid : Int
  ||| The input stream of the spawned process
  input : File
  ||| The output stream of the spawned process
  output : File

Where input is the handle you write to on the Idris side and output is what you read from. There doesn't appear to be a wait primitive - should we keep the pid here? Do we want to add wait and possibly kill to make use of that pid? The output should eof when the process is done, but we might want more control over the process itself.

The popen interface exposed above is higher level and the waiting is done internally to the C library when the special pclose is called.

@mattpolzin
Copy link
Collaborator

mattpolzin commented Apr 12, 2023

RE having use of the PID, even without wait and kill we already have the ability to send signals to it exposed in the base lib.

[EDIT] more accurately, kill is already exposed, but under the name sendSignal.

@dunhamsteve
Copy link
Contributor Author

I switched it over to a Record, added a test case, and got it working on windows.

Copy link
Collaborator

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Nice. Thanks!

@mattpolzin mattpolzin merged commit 9544162 into idris-lang:main Apr 15, 2023
@dunhamsteve dunhamsteve deleted the issue-2935 branch April 15, 2023 14:45
andrevidela added a commit that referenced this pull request Dec 20, 2023
[ re #2944, fix ] Make `popen2` be able to not to leak system resources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants