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

Use process jobs when spawning subprocesses #6529

Closed
wants to merge 1 commit into from

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Feb 6, 2020

Many toolchain tools written for POSIX systems rely on the exec system
call. Unfortunately, it is not possible to implement exec in a
POSIX-compliant manner on Windows. In particular, the semantics of the
exec implementation provided by the widely-used msvcrt C runtime
will cause process's waiting on the exec'ing process to incorrectly
conclude that the process has successfully terminated when in fact it is
still running in another process.

For this reason, the process library exposes the use_process_jobs
flag to use a more strict (although still not POSIX-compliant) mechanism
for tracking process completion. This is explained in this comment [2].

Unfortunately, job support in the process library is currently quite
broken and was only recently fixed [1]. Consequently, we only enable job
object support for process releases >= 1.6.8.

[1] haskell/process#168
[2] https://github.com/haskell/process/blob/master/System/Process.hs#L399

@bgamari bgamari force-pushed the wip/use-process-jobs branch from ab70010 to b6b1ffc Compare February 6, 2020 17:35
Cabal/Distribution/Simple/Utils.hs Outdated Show resolved Hide resolved
Cabal/Distribution/Simple/Utils.hs Outdated Show resolved Hide resolved
Cabal/Distribution/Simple/Utils.hs Show resolved Hide resolved
@phadej
Copy link
Collaborator

phadej commented Feb 6, 2020

Distribution/Simple/Utils.hs:689:22: error:
    Not in scope: type constructor or class ‘CreateProces’
    Perhaps you meant ‘Process.CreateProcess’ (imported from System.Process)
    |
689 | enableProcessJobs :: CreateProces -> CreateProcess

@bgamari bgamari force-pushed the wip/use-process-jobs branch 2 times, most recently from 8085f6a to db1153e Compare February 7, 2020 00:30
Many toolchain tools written for POSIX systems rely on the exec system
call. Unfortunately, it is not possible to implement `exec` in a
POSIX-compliant manner on Windows. In particular, the semantics of the
`exec` implementation provided by the widely-used `msvcrt` C runtime
will cause process's waiting on the `exec`'ing process to incorrectly
conclude that the process has successfully terminated when in fact it is
still running in another process.

For this reason, the `process` library exposes the `use_process_jobs`
flag to use a more strict (although still not POSIX-compliant) mechanism
for tracking process completion. This is explained in this comment [2].

Unfortunately, job support in the `process` library is currently quite
broken and was only recently fixed [1]. Consequently, we only enable job
object support for process releases >= 1.6.8.

[1] haskell/process#168
[2] https://github.com/haskell/process/blob/master/System/Process.hs#L399
@bgamari bgamari force-pushed the wip/use-process-jobs branch from ad5984c to e4db2dc Compare February 7, 2020 00:33
Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

Patch looks a lot nicer now.

@bgamari do you need this for Cabal-3.2 / GHC-8.10, i.e. should this patch be backported?

@phadej
Copy link
Collaborator

phadej commented Feb 7, 2020

There's small failure in

Distribution/Simple/Utils.hs:711:56:
    `Process.delegate_ctlc' is not a (visible) constructor field name

but I can take care of it, if the (restarted) OSX job turns out happy.

@phadej
Copy link
Collaborator

phadej commented Feb 7, 2020

I won't merge this until process-1.6.8.0 is on Hackage, I have no means to verify this patch works when CPP conditionals are satisfied.

@phadej
Copy link
Collaborator

phadej commented Feb 8, 2020

Also, is there a way to see difference between use_process_jobs = True and False, i.e. can we write a test that createProcess behaves correctly?

@bgamari
Copy link
Contributor Author

bgamari commented Feb 12, 2020

What action is required here? I believe the process release is out.

Also, is there a way to see difference between use_process_jobs = True and False, i.e. can we write a test that createProcess behaves correctly?

You could spawn a process which execs another image which sleeps for a while and then performs some observable side effect (e.g. writing to a file). If use_process_jobs works correctly then waitForProcess won't return until the process finishes. This isn't perfectly race-free but I'm afraid it's the best we can do.

@phadej phadej mentioned this pull request Feb 12, 2020
@phadej
Copy link
Collaborator

phadej commented Feb 23, 2020

Merged as part of #6536

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.

2 participants