add PssFreeSnapshot #115
add PssFreeSnapshot #115
Conversation
leehinman
requested review from
rdner,
fearful-symmetry and
AndersonQ
and removed request for
a team
leehinman
added
bug
| defer func() { | ||
| cHandle, _ := syscall.GetCurrentProcess() | ||
| PssFreeSnapshot(cHandle, snapshotHandle) | ||
| }() |
There was a problem hiding this comment.
Only possible issue with putting this in a defer is that if os.Exit is called this won't run. https://pkg.go.dev/os#Exit
If the memory here is allocated in the memory of the current process this won't matter as it'll all be freed on exit, if the memory is held by the kernel then I'm not sure what happens. Hopefully it is also freed because it is associated with the existence of this process in some way.
There was a problem hiding this comment.
It might be safer to call PssFreeSnapshot after calling PssQuerySnapshot but before checking its error code.
There was a problem hiding this comment.
changed to free after query, regardless of query return code.
| @@ -94,6 +94,10 @@ func FetchNumThreads(pid int) (int, error) { | |||
| if err != nil { | |||
| return 0, fmt.Errorf("PssCaptureSnapshot failed: %w", err) | |||
| } | |||
| defer func() { | |||
| cHandle, _ := syscall.GetCurrentProcess() | |||
| PssFreeSnapshot(cHandle, snapshotHandle) | |||
There was a problem hiding this comment.
I think the errors should be returned to caller of FetchNumThreads. If I were using this as a library I would want to know that PssFreeSnapshot is failing.
There was a problem hiding this comment.
thanks, I was going back and forth over that. changed to return error codes from GetCurrentProcess and PssFreeSnapshot
|
Thank you @leehinman |
| @@ -76,3 +77,11 @@ func PssQuerySnapshot(snapshotHandle syscall.Handle, informationClass uint32, bu | |||
| } | |||
| return | |||
| } | |||
|
|
|||
| func PssFreeSnapshot(processHandle syscall.Handle, snapshotHandle syscall.Handle) (err error) { | |||
| r1, _, e1 := syscall.Syscall6(procPssFreeSnapshot.Addr(), 2, uintptr(processHandle), uintptr(snapshotHandle), 0, 0, 0, 0) | |||
There was a problem hiding this comment.
FYI Syscall6 has been deprecated: https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/syscall/dll_windows.go;l=30-31
|
Hi @pierrehilbert Observations:
Build details: Screen Recording: Harshit-windows.-.ec2-100-26-159-124.compute-1.amazonaws.com.-.Remote.Desktop.Connection.2023-12-07.14-14-41.mp4Amol.Win.-.ec2-100-26-253-68.compute-1.amazonaws.com.-.Remote.Desktop.Connection.2023-12-07.14-14-00.mp4Amol.Windows.10GB.RAM.-.ec2-3-83-217-17.compute-1.amazonaws.com.-.Remote.Desktop.Connection.2023-12-07.14-12-35.mp4Hence we are marking this as [QA:Validated]. Please let us know if we are missing anything here. |
What does this PR do?
Frees the memory that is allocated when PssCaptureSnapshot is called.
Why is it important?
memory leak
Checklist
CHANGELOG.mdAuthor's Checklist
Related issues
Testing