os: export errFinished as ErrProcessDone #39444

inyutin · 2020-06-06T15:47:31Z

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:

go/src/os/exec_unix.go

Line 64 in fca286b

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 · 2020-06-07T01:36:14Z

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 · 2020-06-07T08:23:38Z

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 · 2020-06-08T21:30:28Z

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

rsc · 2020-06-10T17:34:18Z

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 · 2020-06-17T17:12:36Z

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 · 2020-06-18T06:24:42Z

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 · 2020-06-18T15:56:40Z

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 · 2020-06-24T17:24:50Z

@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 · 2020-07-08T17:16:46Z

No responses for ErrProcessDone, but it seems like people are in general OK with exporting the error.
Given the lack of any objections, it sounds like this is a likely accept.

rsc · 2020-07-15T17:07:50Z

No change in consensus, so accepted.

gopherbot · 2020-07-16T17:51:52Z

Change https://golang.org/cl/242998 mentions this issue: os: export errFinished as ErrProcessDone

gopherbot · 2020-12-03T10:11:01Z

Change https://golang.org/cl/274477 mentions this issue: doc/go1.16: document os package changes

For #39444 For #40700 Fixes #42908 Change-Id: Idae35adecd79e9d7d207f9d78cb009a980e5c8a9 Reviewed-on: https://go-review.googlesource.com/c/go/+/274477 Trust: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Russ Cox <rsc@golang.org>

ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 7, 2020

ianlancetaylor changed the title ~~Why os.signal return os.errFinished instead of syscall.ESRCH?~~ proposal: os: export errFinished Jun 8, 2020

ianlancetaylor removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 8, 2020

gopherbot added this to the Proposal milestone Jun 8, 2020

ianlancetaylor added the Proposal label Jun 8, 2020

ianlancetaylor added this to Incoming in Proposals (old) Jun 8, 2020

rsc moved this from Incoming to Active in Proposals (old) Jun 10, 2020

rsc mentioned this issue Jun 10, 2020

proposal: runtime: add way for applications to respond to GC backpressure #29696

Open

andybons mentioned this issue Jun 10, 2020

proposal: review meeting minutes #33502

Open

rsc changed the title ~~proposal: os: export errFinished~~ proposal: os: make errFinished satisfy errors.Is(..., ErrNotExist) Jun 17, 2020

vinu2003 mentioned this issue Jun 23, 2020

service on docker with Ctrl-C: error stopping tunnel: stopping ssh tunnel: os: process already finished kubernetes/minikube#8511

Open

rsc changed the title ~~proposal: os: make errFinished satisfy errors.Is(..., ErrNotExist)~~ proposal: os: export errFinished as ErrProcessDone Jun 24, 2020

rsc moved this from Active to Likely Accept in Proposals (old) Jul 8, 2020

rsc added the Proposal-FinalCommentPeriod label Jul 8, 2020

rsc moved this from Likely Accept to Accepted in Proposals (old) Jul 15, 2020

rsc modified the milestones: Proposal, Backlog Jul 16, 2020

rsc changed the title ~~proposal: os: export errFinished as ErrProcessDone~~ os: export errFinished as ErrProcessDone Jul 16, 2020

rsc added Proposal-Accepted and removed Proposal-FinalCommentPeriod labels Jul 16, 2020

gopherbot closed this as completed in f14119b Oct 31, 2020

dmitshur modified the milestones: Backlog, Go1.16 Nov 1, 2020

bcmills mentioned this issue Nov 2, 2020

os: ErrProcessDone not defined on windows #42311

Closed

dmitshur mentioned this issue Dec 28, 2020

api: audit for Go 1.16 go1.16.txt #43407

Closed

golang locked and limited conversation to collaborators Dec 3, 2021

gopherbot added the FrozenDueToAge label Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: export errFinished as ErrProcessDone #39444

os: export errFinished as ErrProcessDone #39444

inyutin commented Jun 6, 2020

ianlancetaylor commented Jun 7, 2020

inyutin commented Jun 7, 2020

ianlancetaylor commented Jun 8, 2020

rsc commented Jun 10, 2020

rsc commented Jun 17, 2020

networkimprov commented Jun 18, 2020

bcmills commented Jun 18, 2020

rsc commented Jun 24, 2020

rsc commented Jul 8, 2020

rsc commented Jul 15, 2020

gopherbot commented Jul 16, 2020

gopherbot commented Dec 3, 2020

os: export errFinished as ErrProcessDone #39444

os: export errFinished as ErrProcessDone #39444

Comments

inyutin commented Jun 6, 2020

ianlancetaylor commented Jun 7, 2020

inyutin commented Jun 7, 2020

ianlancetaylor commented Jun 8, 2020

rsc commented Jun 10, 2020

rsc commented Jun 17, 2020

networkimprov commented Jun 18, 2020

bcmills commented Jun 18, 2020

rsc commented Jun 24, 2020

rsc commented Jul 8, 2020

rsc commented Jul 15, 2020

gopherbot commented Jul 16, 2020

gopherbot commented Dec 3, 2020