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

process_group for v2 #259

Open
klemens-morgenstern opened this issue Jun 11, 2022 · 12 comments
Open

process_group for v2 #259

klemens-morgenstern opened this issue Jun 11, 2022 · 12 comments
Assignees

Comments

@klemens-morgenstern
Copy link
Collaborator

See klemens-morgenstern/boost-process#216

@klemens-morgenstern klemens-morgenstern self-assigned this Jun 11, 2022
@klemens-morgenstern klemens-morgenstern changed the title [link] Enable ability to set the job object to kill children processes if the parent dies #216 process_group for v2 Jun 11, 2022
@klemens-morgenstern
Copy link
Collaborator Author

@kannon92 @emmenlau

I think the correct solution here is a process-group. I haven't added that to v2 yet, because it's not trivial to do the async_wait, but it might solve your issues. I'll use the issue to spec out a potential process_group interface, and we'll see if it matches your requirements.

The process group would have a lead process (which doesn't mean as much on windows, but it's the source of the ::gid on linux), so you could use it like this:

Note that the process_group would be a console process group & job object on windows. so it can be SIGINT-ed.

This is what I envision:

asio::io_context ctx;
v2::process_group proc_grp{ctx, "my_proc", {}};

Now this should address klemens-morgenstern/boost-process#216 already, since all sub process of my_proc would be in the same process-group, thus reaped when the process group is done.

However, if one wanted to add more processes it could be done like so:

pid_type pid = proc_grp.insert("other-proc", {});
pid_type pid2 = proc_grp.insert("other-proc", {});


proc_grp.interrupt();//send SIGINT to all subprocesses.

// wait for one process to exit - also should have async_wait_one()
auto [pid_exited, int] = proc_grp.wait_one();
// wait for all processes to exit
proc_grp.wait_all();.

@emmenlau
Copy link

Dear @klemens-morgenstern , this sounds very promising! Do I understand correctly that as a default, all processes would start in the same process group? Or is that something that the user has to explicitly do? It may be helpful if all processes default to the same process group, so that unsuspecting users do not encounter problems of un-reaped processes on application end.

I think this is perfectly in line with the behavior we need, and all the better if the process group can be extended with other processes later!

@egorpugin
Copy link
Contributor

egorpugin commented Jun 11, 2022

@klemens-morgenstern

Process group is also used for killing children when current process dies.
Grep job here https://github.com/libuv/libuv/blob/v1.x/src/win/process.c#L63=

Like when I spawn some build jobs, they all die on control-c with main process.
So the interface could be also like

asio::io_context ctx;
v2::process_group proc_grp{ctx}; // new proc group
auto p1 = process{ctx, "some_proc", {}, proc_grp};
auto p2 = process{ctx, "some_proc", {}, proc_grp};

auto p3 = process{ctx, "some_proc", {}, v2::default_proc_grp}; // static var, this process' group/job

Upd.:
As for waiting all job's processes to terminate, possible solution can be found here https://devblogs.microsoft.com/oldnewthing/20130405-00/?p=4743

@klemens-morgenstern
Copy link
Collaborator Author

Oh ok, you think defaulting to a default group is a good idea? Shouldn't that be controlled by the parent process ? I.e. the grandparent of the started process?

Egor what you proposed is similar to what v1 does with groups, but it doesn't work without some weird constructs, due to waitpid reaping the child process. I.e. if I already waited for a given process through the group it'll be removed from the porocess list and I get a no child process error e.g.:

asio::io_context ctx;
v2::process_group proc_grp{ctx}; // new proc group
auto p1 = process{ctx, "some_proc", {}, proc_grp};

proc_grp.wait_one();
p1.wait(); // throws no such process.

Which is why I like the idea of emplacing the process into the group more. I would support a detach function (i.e. steal a process from a group) but I don't think the winapi supports that.

@egorpugin
Copy link
Contributor

Oh ok, you think defaulting to a default group is a good idea?

I mean how to add new process to the current job with your proposed interface?

Example (windows terms):

Current process does not belong to any job. I.e. parent (grandparent for our children) did not create any jobs etc.
To kill all our children when we die, we need 1) create job, 2) add children handles to it.

How will this look like?


Oh ok, you think defaulting to a default group is a good idea? Shouldn't that be controlled by the parent process ? I.e. the grandparent of the started process?

On windows each process may be a part of several jobs. So, when grandparent dies, current process dies, so all children die. Looks ok.
But I'm not sure how this works on posix.

@klemens-morgenstern
Copy link
Collaborator Author

Interesting question , I know on linux you are by default in the same GID - is that not the case on windows?

I thought you need to setJOB_OBJECT_LIMIT_BREAKAWAY_OK explicitly to move to a different job, thus it would be same default? Am I misunderstanding something?

@egorpugin
Copy link
Contributor

Probably since we have access to native handles, user can setup needed behavior on windows himself.

@emmenlau
Copy link

emmenlau commented Jun 12, 2022

Probably since we have access to native handles, user can setup needed behavior on windows himself.

It would be great if the "usually expected" behavior would be set up as simple as possible.

For me, a good default would be that all processes end when the parent (that started them) ends. I think this would be a useful default for many users. Any other behavior seem it could be quite unexpected, and possibly hurtful to users.

@egorpugin
Copy link
Contributor

For me, a good default would be that all processes end when the parent (that started them) ends.

libuv uses that.

@klemens-morgenstern
Copy link
Collaborator Author

But then you'd detached from the current process everytime. So if you process gets put in a group by it's caller, and then puts it's child processed in a different group, you'd get weird behaviour for other users. That's why I usually stick to the OS defaults.

Have you guys only used groups to avoid zombies & terminating? Of what utility are group.wait functions?

@klemens-morgenstern
Copy link
Collaborator Author

I've been working trying to figure out how to make job-object on windows work, with little success. I think I'll tall it, and just provide pgid initializers for posix. The posix API is easy enough to use alone, and the windows job object api is to messy to be useful I fear.

is anyone using the the windows stuff?

@kannon92
Copy link

I've been working trying to figure out how to make job-object on windows work, with little success. I think I'll tall it, and just provide pgid initializers for posix. The posix API is easy enough to use alone, and the windows job object api is to messy to be useful I fear.

is anyone using the the windows stuff?

Sorry for not responding. I no longer work for the employer that was using this. We were primarily interested in boost process because of windows and Linux support. We were aiming to support this in both OS and did test it in both actually.

So windows was used ;) our initial rollout of the project was completely focused on windows

kou added a commit to apache/arrow that referenced this issue Sep 3, 2024
### Rationale for this change

`boost/process/*.hpp` are deprecated since Boost 1.86. And it seems that it also adds backward incompatible change. We need to use `boost/process/v2/*.hpp` instead.

### What changes are included in this PR?

This introduces `arrow::util::Process` for testing. It wraps boost/process/ API. So we don't need to use boost/process/ API directly in our tests.

We still use the v1 API on Windows because the v2 API doesn't process group and we don't have a workaround for it on Windows. If GCS's testbench doesn't use multiple processes, we can use the v2 API on Windows because we don't need to use process group in our use case.

See also:
* The v2 API and process group: boostorg/process#259
* GCS's testbench and multiple processes: googleapis/storage-testbench#669

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: #43746

Lead-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
mapleFU pushed a commit to mapleFU/arrow that referenced this issue Sep 3, 2024
### Rationale for this change

`boost/process/*.hpp` are deprecated since Boost 1.86. And it seems that it also adds backward incompatible change. We need to use `boost/process/v2/*.hpp` instead.

### What changes are included in this PR?

This introduces `arrow::util::Process` for testing. It wraps boost/process/ API. So we don't need to use boost/process/ API directly in our tests.

We still use the v1 API on Windows because the v2 API doesn't process group and we don't have a workaround for it on Windows. If GCS's testbench doesn't use multiple processes, we can use the v2 API on Windows because we don't need to use process group in our use case.

See also:
* The v2 API and process group: boostorg/process#259
* GCS's testbench and multiple processes: googleapis/storage-testbench#669

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#43746

Lead-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Sep 6, 2024
### Rationale for this change

`boost/process/*.hpp` are deprecated since Boost 1.86. And it seems that it also adds backward incompatible change. We need to use `boost/process/v2/*.hpp` instead.

### What changes are included in this PR?

This introduces `arrow::util::Process` for testing. It wraps boost/process/ API. So we don't need to use boost/process/ API directly in our tests.

We still use the v1 API on Windows because the v2 API doesn't process group and we don't have a workaround for it on Windows. If GCS's testbench doesn't use multiple processes, we can use the v2 API on Windows because we don't need to use process group in our use case.

See also:
* The v2 API and process group: boostorg/process#259
* GCS's testbench and multiple processes: googleapis/storage-testbench#669

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#43746

Lead-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
khwilson pushed a commit to khwilson/arrow that referenced this issue Sep 14, 2024
### Rationale for this change

`boost/process/*.hpp` are deprecated since Boost 1.86. And it seems that it also adds backward incompatible change. We need to use `boost/process/v2/*.hpp` instead.

### What changes are included in this PR?

This introduces `arrow::util::Process` for testing. It wraps boost/process/ API. So we don't need to use boost/process/ API directly in our tests.

We still use the v1 API on Windows because the v2 API doesn't process group and we don't have a workaround for it on Windows. If GCS's testbench doesn't use multiple processes, we can use the v2 API on Windows because we don't need to use process group in our use case.

See also:
* The v2 API and process group: boostorg/process#259
* GCS's testbench and multiple processes: googleapis/storage-testbench#669

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#43746

Lead-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
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

4 participants