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

proposal: os: export errFinished as ErrProcessDone #39444

Open
inyutin opened this issue Jun 6, 2020 · 8 comments
Open

proposal: os: export errFinished as ErrProcessDone #39444

inyutin opened this issue Jun 6, 2020 · 8 comments
Labels
Projects
Milestone

Comments

@inyutin
Copy link

@inyutin inyutin commented Jun 6, 2020

In Linux, kill() can return there possible errors:

[EINVAL]
The value of the sig argument is an invalid or unsupported signal number.

[EPERM]
The process does not have permission to send the signal to any receiving process.

[ESRCH]
No process or process group can be found corresponding to that specified by pid.

This is how os.signal function works in Go:

func (p *Process) signal(sig Signal) error {

func (p *Process) signal(sig Signal) error {
	if p.Pid == -1 {
		return errors.New("os: process already released")
	}
	if p.Pid == 0 {
		return errors.New("os: process not initialized")
	}
	p.sigMu.RLock()
	defer p.sigMu.RUnlock()
	if p.done() {
		return errFinished
	}
	s, ok := sig.(syscall.Signal)
	if !ok {
		return errors.New("os: unsupported signal type")
	}
	if e := syscall.Kill(p.Pid, s); e != nil {
		if e == syscall.ESRCH {
			return errFinished
		}
		return e
	}
	return nil
}

What it returns private errFinished instead of public ESRCH ?
Because of that I can't write some custom logic like this:

err := process.Kill()
if err == syscall.ESRCH {
  // process already finished do something
}

It's okay that this error can have custom name, but why it private? Why errFinished, but not ErrFinished?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 7, 2020

It returns errFinished because ESRCH doesn't really seem right if the process has already exited. Also see #7658.

If you really need to distinguish these cases, which means that you are already doing Unix-specific things, I recommend that you use syscall.Kill(process.Pid, syscall.SIGKILL).

@inyutin
Copy link
Author

@inyutin inyutin commented Jun 7, 2020

It returns errFinished because ESRCH doesn't really seem right if the process has already exited. Also see #7658.

If you really need to distinguish these cases, which means that you are already doing Unix-specific things, I recommend that you use syscall.Kill(process.Pid, syscall.SIGKILL).

Thank you for answer, that's all make a point. I wish to use os.signal, because it already have lot's of useful logic like rw-locks and pid checking. If I use raw syscall.Kill(process.Pid, syscall.SIGKILL), that I need to implement the same logic on my own, in my application.

Are there any disadvantages from making errFinished public?

@ianlancetaylor ianlancetaylor changed the title Why os.signal return os.errFinished instead of syscall.ESRCH? proposal: os: export errFinished Jun 8, 2020
@gopherbot gopherbot added this to the Proposal milestone Jun 8, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 8, 2020

OK, I'll turn this into a proposal to export errFinished.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 8, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

I'm a little hesitant about exporting errFinished under that name, since it's not a general "something finished". I'm also having a hard time coming up with a better name. The wait man page says "wait for process termination" so we could use ErrProcessTerminated but that's not in keeping with the other names in os (too long, and also too specific).

@ianlancetaylor pointed out maybe we could use ErrNotExist, or perhaps we could keep using errFinished but make errors.Is(errFinished, ErrNotExist) true.

Thoughts?

@rsc rsc moved this from Incoming to Active in Proposals Jun 10, 2020
@rsc rsc changed the title proposal: os: export errFinished proposal: os: make errFinished satisfy errors.Is(..., ErrNotExist) Jun 17, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jun 17, 2020

As I noted last week, it seems like the least API surface would be to make errFinished be errors.Is(..., ErrNotExist). Retitled to that. Does anyone object to that?

@networkimprov
Copy link

@networkimprov networkimprov commented Jun 18, 2020

Does that mean os.IsNotExist(err) == true for os.errFinished? That could be surprising, since the former is for filesystem errors. In many situations, os.IsNotExist(err) is a normal condition.

I'd expect signal and other process-related APIs to return errors specific to processes.

cc @alexbrainman @bcmills @aclements

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 18, 2020

Thinking about method calls in grammatical terms, I expect os.ErrNotExist to indicate “the direct object (of the verb) does not exist”:

  • os.ErrNotExist from Stat(f) indicates “could not Stat [verb] f [direct object] because f does not exist.”
  • os.ErrNotExist from Open(f) indicates “could not Open [verb] f [direct object] because f does not exist.”

The proposed usage would be consistent with that, but requires the reader to treat the name of the method as the verb (as they should, but may not always do).

  • “Could not Signal [verb] p [direct object] with sig [object of preposition] because p does not exist.”
  • “Could not send [verb] Signal sig [direct object] to p [indirect object] because sig does not exist.”

On the other hand, the error text of os.ErrNotExist is specifically documented as file does not exist, and as @networkimprov notes, there is no file involved in Signal unless you consider the PID itself to be a file. I would not expect most users to make that association.

I think a more general standardized error for “[direct object] does not exist” could be useful, but I agree that, because of its specific text, os.ErrNotExist is not quite that error.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 24, 2020

@bcmills, it's not as clear cut as you say. I wouldn't be surprised if f.Stat() could return ErrNotExist in certain cases (those cases do not include "local file on Unix"). In any event, it's likely clear to the caller that the signal itself does exist.

On the other hand, ErrNotExist.String() is "file does not exist", so that's a show-stopper.

ErrProcessDone? (at least we have established Done in context, and it's shorter than Finished)

@rsc rsc changed the title proposal: os: make errFinished satisfy errors.Is(..., ErrNotExist) proposal: os: export errFinished as ErrProcessDone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.