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

NewProcess does not return an error if process does not exist on Windows #729

Closed
2 of 5 tasks
dhaavi opened this issue Jul 23, 2019 · 5 comments · Fixed by #735
Closed
2 of 5 tasks

NewProcess does not return an error if process does not exist on Windows #729

dhaavi opened this issue Jul 23, 2019 · 5 comments · Fixed by #735
Assignees

Comments

@dhaavi
Copy link

dhaavi commented Jul 23, 2019

Describe the bug

The docs for NewProcess state that An error will be returned if the process does not exist.
Running on Windows 10, NewProcess does not return an error if called with an invalid PID.
(Works fine on Linux 4.9)

To Reproduce

package main

import (
	"fmt"

	"github.com/shirou/gopsutil/process"
)

func main() {
	_, err := process.NewProcess(9999)
	if err == nil {
		fmt.Println("process exists!")
	}
}

Expected behavior
NewProcess should return an error.

Environment (please complete the following information):

  • Windows: Microsoft Windows [Version 10.0.17763.615]
  • Linux: Linux 74b9ced6499d 4.9.125-linuxkit #1 SMP Fri Sep 7 08:20:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
  • Mac OS: [paste the result of sw_vers and uname -a
  • FreeBSD: [paste the result of freebsd-version -k -r -u and uname -a]
  • OpenBSD: [paste the result of uname -a]

Additional context
Library version:

  name = "github.com/shirou/gopsutil"
  revision = "4c8b404ee5c53b04b04f34b1744a26bf5d2910de"
  version = "v2.19.6"

Cross-compiling for Windows on Linux using GOOS=windows go build in a dir with only the file pasted above.

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.gocache"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/user/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go-1.12"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.12/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build008209370=/tmp/go-build -gno-record-gcc-switches"

Workaround

package main

import (
	"fmt"

	"github.com/shirou/gopsutil/process"
)

func main() {
	p, err := process.NewProcess(9999)
	if err == nil {
		_, err = p.Name() // edited
		if err == nil {
			fmt.Println("process exists!")
		}
	}
}
@Lomanic Lomanic self-assigned this Jul 23, 2019
@Lomanic
Copy link
Collaborator

Lomanic commented Jul 23, 2019

This check is only present on Linux currently (checking if /proc/$PID exists). Work in #713 will make it viable (without destroying performances as it would have before on some platforms, for example on Darwin) to check PID existence each time you call process.NewProcess.

@Lomanic
Copy link
Collaborator

Lomanic commented Jul 23, 2019

Trying to call process.Status() on Windows is a bad idea to check if process exists on Windows as it is not implemented and returns a common.ErrNotImplementedError.

@dhaavi
Copy link
Author

dhaavi commented Jul 23, 2019

I see. Could you then update the documentation to reflect this behavior? The windows version of NewProcess is undocumented. (I must admit, I did not check it before raising the issue)

Ha! Good Catch! Thanks for that.
What would you recommend as a workaround until this can be fixed with #713?

@Lomanic
Copy link
Collaborator

Lomanic commented Jul 23, 2019

The windows version of NewProcess is undocumented.

Documentation is only present on linux because it's godoc.org default GOOS (last time I checked there was no way to change GOOS via URL parameter on godoc.org) and because we have the same documentation for all the platforms.

You can check for process existence with process.PidExists(). This will be slow with current implementation of process.Pids() on darwin though (and on others that may call ps to list processes).

Could you then update the documentation to reflect this behavior?

This will be fixed with #713 being merged and the test for process existence being added to process.NewProcess() ; it's been like this for quite a long time, there is little value for me to modify the documentation when it will be fixed in the coming weeks.

@dhaavi
Copy link
Author

dhaavi commented Jul 24, 2019

there was no way to change GOOS via URL parameter on godoc.org

I know - that is why i checked the source code. I am really annoyed by this behavior of godoc.org.

there is little value for me to modify the documentation when it will be fixed in the coming weeks.

Perfect. Wasn't aware of that timeline.

Lomanic added a commit to Lomanic/gopsutil that referenced this issue Aug 1, 2019
Lomanic added a commit that referenced this issue Aug 10, 2019
[process] Fix #729 check process existence in NewProcess()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants