search

shirou / gopsutil

psutil for golang
Other
10.56k stars 1.58k forks source link

misleading function name in process_windows.go #1086

Closed gekigek99 closed 3 years ago

gekigek99 commented 3 years ago

I was researching a way to suspend a process knowing the pid of a process and ended up here:

https://github.com/shirou/gopsutil/blob/master/process/process_windows.go#L578-L608

It appears that the context is non used and so it's misleading to name the function SuspendWithContext(ctx context.Context) and ResumeWithContext(ctx context.Context) (even more by setting the parameter ctx context.Context)

Maybe the rename of the function and removal of the unused parameter (or at least an addition in the documentation) should be considered.

Thank you for the consideration

Lomanic commented 3 years ago

We won't remove or rename this function as it's the same API on all OSes.

The context argument is not used on all/most *WithContext functions on all OSes, we probably would have to implement this way on all these functions https://github.com/shirou/gopsutil/blob/8dd42707e2221e26bdb87c095aeaab4e7d3a8487/internal/common/common_windows.go#L117-L135

gekigek99 commented 3 years ago

oh ok I understand. It was just to highlight this fact nothing more.

So you'll probably implement the usage of ctx context.Context in the future? (for different functions on all os?)

I wanted also to point out that apparently, using windows.OpenProcess(windows.PROCESS_SUSPEND_RESUME, false, uint32(p.Pid)) might return an invalid/unusable handle as said here in the comments

Could you explain why in this case windows.OpenProcess can be used safely?

shirou commented 3 years ago

As @Lomanic said, ctx is used on different platforms, and others that do not, but all functions should have the same signature for all platforms. So I have prepared ctx for all functions. but yes, only a few functions uses ctx currently.

Could you explain why in this case windows.OpenProcess can be used safely?

It seems golang.org/x/sys/windows problem, and we have not noticed. PR is always welcome!

gekigek99 commented 3 years ago

oh ok if you say that you have no problem with that then It should not be a problem even for me...

I guess that the issue that might lead to problems is that pids can be reused and so if a process exit and an other takes the same pid, then this might lead to bugs

Lomanic commented 3 years ago

We have the IsRunning() function to use for the end-user to protect against PID reuse somewhat, but again there's no guarantee that after they called this function the process PID is not being reused.

I don't think we can do much in gopsutil as we only work on PIDs.

gekigek99 commented 3 years ago

Thank you for the explanation.

I think I can close the issue now