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

Remove dependency on the process package #23

Open
harendra-kumar opened this issue Jul 14, 2021 · 3 comments
Open

Remove dependency on the process package #23

harendra-kumar opened this issue Jul 14, 2021 · 3 comments

Comments

@harendra-kumar
Copy link
Member

We should directly depend on unix and Win32. Also, process depends on some C code we should try to remove that completely or reduce it.

@twitu
Copy link
Collaborator

twitu commented Jul 28, 2022

This issue is depends on fixing #34 #51

Fixing builds for native led to a tests failing as documented in #51. The possible reason is covered in a comment.

After discussion, the current cause of the bug appears to be how the process is being cloned in the ghc runtime. It is likely that ghc RTS threads implemented using pthreads, is using clone call with flags that do not copy over the file descriptors.

With this assumption the sequence of steps that leads to the error are these -

  1. Parent process creates a pipe
  2. File descriptors are updated in green thread 1 (GT 1)
  3. Parent process "clones" a child process on GT 1
  4. Child process is moved to GT 2 because of scheduling
  5. GT2 does not have fd structures of pipe created in GT 1. This is related to how threads have been implemented in GHC RTS.
  6. Child process tries to close fds that do not exist

This is an intermittent issue because sometimes step 4 does not happen. This error also does not occur on using the process package because the library clones parent and child process using a c ffi function, somehow GHC makes sure process created through ffi stays on the same GT. Another point that validates the above cause is that tests pass when configured to run with 1 thread only.

Originally posted by @twitu in #51 (comment)

@harendra-kumar
Copy link
Member Author

You can try using a bound thread so that the entire computation is done in the same OS thread. Wrap createProc' in forkOS, and pass the results via an MVar. That should take care of this issue.

@twitu
Copy link
Collaborator

twitu commented Aug 13, 2022

Very weird. Unbound thread is not making a difference. The error is consistently reproducing now. Even with with 1 thread!!

cabal test --test-options="--match \"/Streamly.System.Process/pipeBytes/pipeBytes tr = map toUpper/\"" -f use-native +RTS -N1 -RTS

I've added some additional logs that print out the fds but that doesn't help much.

parent process 1545416
child process 1545416
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62
child input action doing
closing parent fd58
Test.System.Process: closeFd: invalid argument (Bad file descriptor)
    pipeBytes tr = map toUpper [✘]

Notice that parent and child process ids are the same even though the child process is printing it's id in the exec action after forkProcess. And the fds printed in child action does contain fd 58. Below is the log when the test passes successfully.

parent process 1545389
child process 1545389
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62
child input action doing
closing parent fd58
closed parent fd58
duplicating child to fd(57,0)
duplicated child to fd(57,0)
closing child57
closed child57
child input action done
child output action doing
closing parent fd59
closed parent fd59
duplicating child to fd(60,1)
duplicated child to fd(60,1)
closing child60
closed child60
child output action done

Logging logic is pushed to latest commit (ec53056) in use-native

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants