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

Export runInteractiveProcess_lock on Posix systems #154

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

thomasjm
Copy link
Contributor

@thomasjm thomasjm commented Sep 16, 2019

This PR is to help fix merijn/posix-pty#15.

Summary: on Posix systems, System.Process must take certain precautions before using the fork system call. These include altering global state by calling blockUserSignals and stopTimer() before forking; see https://github.com/haskell/process/blob/master/cbits/runProcess.c#L137.

However, System.Process is not the only library that might need to use fork. System.Posix.Pty is another such library -- it uses the forkpty call (which internally does fork) to create a process attached to a pseudo-terminal. Thus System.Posix.Pty also needs to take these precautions.

As a result, we need to ensure mutual exclusion between System.Posix.Pty fork calls and System.Process fork calls, or else they stomp on each other's changes to global state. The only way I can see to do this is to use the same lock, so this PR exports it. I made a PR on posix-pty to leverage it here: merijn/posix-pty#16.

Of course, this feels like a bit of a band-aid since it only ensures that interleaving these two libraries is safe. Other libraries that might want to fork will still run into problems interleaving with these libraries. It would be nice to get some GHC expert attention on this but maybe that's an issue for another tracker.

@snoyberg
Copy link
Collaborator

I don't feel strongly either way, but overall I'm concerned that there may be undocumented invariants, and exposing this locking primitive will end up encouraging misuse.

@thomasjm
Copy link
Contributor Author

Yes, agreed this is not pretty but it seems to be forced by the design of the RTS. If there were some facility provided by the language itself to ensure that an FFI fork call is safe, then libraries wouldn't have to come up with their own locking schemes.

As it stands, other libraries can't safely fork if that operation might get interleaved with a System.Process call, and I think we should fix that ASAP. For me it produced a quite nasty intermittent livelock bug. Surfacing this flaw might help other people avoid the same while a more principled fix is investigated.

It seems like a reasonable compromise to put it in System.Process.Internal, no? That kind of communicates the sketchiness of the fix and allows us to remove it someday later.

@snoyberg
Copy link
Collaborator

Fair enough. In that case, can you add a changelog entry, and some Haddocks on the lock explaining what it is and why it's exposed, possibly with a link to this PR?

@thomasjm
Copy link
Contributor Author

Done!

changelog.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Thank you!

@snoyberg snoyberg merged commit 8839ad5 into haskell:master Sep 26, 2019
@thomasjm
Copy link
Contributor Author

Np! Any chance I could get you to cut a Hackage release @snoyberg ?

@snoyberg
Copy link
Collaborator

snoyberg commented Oct 2, 2019

Done!

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

Successfully merging this pull request may close these issues.

Forking can interact with runtime to cause livelock (100% cpu usage) or freezing
2 participants