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

bg behaves differently depending on order of arguments if any are invalid #3909

Closed
zanchey opened this Issue Mar 24, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@zanchey
Member

zanchey commented Mar 24, 2017

Let's say you have some jobs:

> sleep 30 &; sleep 50 &; sleep 90 &
> kill -SIGSTOP %sleep
Job 3, 'sleep 90 &' has stopped
Job 2, 'sleep 50 &' has stopped
Job 1, 'sleep 30 &' has stopped
> jobs
Job	Group	State	Command
3	35465	stopped	sleep 90 &
2	35464	stopped	sleep 50 &
1	35463	stopped	sleep 30 &

For whatever reason, I decide to background them all with manually-specified PGIDs. Unfortunately, I fat-finger one of the PGIDs. What is the difference between these commands?

> bg 35463 a35464 35465
> bg 35465 a35464 35463

If you said each one only backgrounds one process, and it's different between the two, you are correct!

I don't think this is sensible. Either all valid PGIDs should be backgrounded, or none should. Order should not matter.

(The new disown builtin disowns all valid PGIDs, but returns an error if any of the PGIDs are invalid.)

@zanchey zanchey referenced this issue Mar 24, 2017

Merged

Implement disown builtin #3906

3 of 3 tasks complete
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 24, 2017

Contributor

My apologies, @zanchey, for what follows but I think it is important. I hate to be pedantic but by definition these jobs are already backgrounded since you can type a command. Which means those jobs are not in the foreground. What is really being discussed is whether one, some, or none of the jobs should be resumed in the background in light of a bg command that includes an invalid job ID. I'm being pedantic because it seems like some of the issues involving job management are due to not being sufficiently precise about these terms.

Commands like bg and disown have an inherent ambiguity. Any of the jobs may have changed state at any point prior to the command attempting to modify their state. The problem is magnified if the command accepts multiple job IDs. The right thing to do, in my opinion, is what you said in your final sentence. That is, modify the state of all valid job IDs and return an error status if one or more job IDs could not be modified.

Contributor

krader1961 commented Mar 24, 2017

My apologies, @zanchey, for what follows but I think it is important. I hate to be pedantic but by definition these jobs are already backgrounded since you can type a command. Which means those jobs are not in the foreground. What is really being discussed is whether one, some, or none of the jobs should be resumed in the background in light of a bg command that includes an invalid job ID. I'm being pedantic because it seems like some of the issues involving job management are due to not being sufficiently precise about these terms.

Commands like bg and disown have an inherent ambiguity. Any of the jobs may have changed state at any point prior to the command attempting to modify their state. The problem is magnified if the command accepts multiple job IDs. The right thing to do, in my opinion, is what you said in your final sentence. That is, modify the state of all valid job IDs and return an error status if one or more job IDs could not be modified.

@krader1961 krader1961 added this to the fish-future milestone Mar 24, 2017

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 24, 2017

Member

Like I said in a comment in #3906, this is what I'd prefer:

If something that is not an id is specified (i.e. something that isn't a positive integer), that should be a fatal error. No job should be disowned

If a valid-but-nonexistent id is specified, it should just be skipped. The other jobs should still be disowned.

The reason for the latter is that the job may have just finished, and the user shouldn't have to rush to get something disowned.

This is simple and predictable. bg 35463 a35464 35465 would not touch any job (because a35464 is not a PGID, while bg 35463 35465 would touch any of the two that are still running, ensuring that any job is either bg'd or finished.

Member

faho commented Mar 24, 2017

Like I said in a comment in #3906, this is what I'd prefer:

If something that is not an id is specified (i.e. something that isn't a positive integer), that should be a fatal error. No job should be disowned

If a valid-but-nonexistent id is specified, it should just be skipped. The other jobs should still be disowned.

The reason for the latter is that the job may have just finished, and the user shouldn't have to rush to get something disowned.

This is simple and predictable. bg 35463 a35464 35465 would not touch any job (because a35464 is not a PGID, while bg 35463 35465 would touch any of the two that are still running, ensuring that any job is either bg'd or finished.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Mar 24, 2017

@faho's suggestion is very good.

  1. Most shell scripts would be superior if they were idempotent, and these semantics make that property more easy to come by.

  2. I'm a big fan of set -e and friends (a whole other topic :)), and failing on invalid syntax is very much in that fail-fast spirit.

fail-fast and idempotency have some tension, and @faho threads the needle perfectly in my opinion.

Ericson2314 commented Mar 24, 2017

@faho's suggestion is very good.

  1. Most shell scripts would be superior if they were idempotent, and these semantics make that property more easy to come by.

  2. I'm a big fan of set -e and friends (a whole other topic :)), and failing on invalid syntax is very much in that fail-fast spirit.

fail-fast and idempotency have some tension, and @faho threads the needle perfectly in my opinion.

@krader1961 krader1961 removed the RFC label Mar 29, 2017

faho added a commit to faho/fish-shell that referenced this issue Apr 2, 2017

Improve `bg` argument handling
- Error out if anything that is not a PID is given

- Otherwise background all matching existing jobs, even if not all
  PIDs exist

Fixes #3909.

@faho faho referenced this issue Apr 2, 2017

Closed

Bg arguments #3932

3 of 3 tasks complete

@faho faho closed this in 3edb7d5 Apr 4, 2017

@faho faho modified the milestones: 2.6.0, fish-future Apr 10, 2017

develop7 added a commit to develop7/fish-shell that referenced this issue Apr 17, 2017

Improve `bg` argument handling
- Error out if anything that is not a PID is given

- Otherwise background all matching existing jobs, even if not all
  PIDs exist (but print a message for the non-existing ones)

Fixes #3909.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment