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.

Sign up for GitHub

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

add PssFreeSnapshot #115

Merged
merged 2 commits into from
Nov 21, 2023
Merged

add PssFreeSnapshot #115

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1eab78c
add PssFreeSnapshot
leehinman Nov 16, 2023
5c9e99f
address feedback
leehinman Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions metric/system/process/process_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I was going back and forth over that. changed to return error codes from GetCurrentProcess and PssFreeSnapshot

}()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be safer to call PssFreeSnapshot after calling PssQuerySnapshot but before checking its error code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to free after query, regardless of query return code.


info := PssThreadInformation{}
buffSize := unsafe.Sizeof(info)
Expand Down
2 changes: 2 additions & 0 deletions metric/system/process/syscall_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package process
// - https://learn.microsoft.com/en-us/previous-versions/windows/desktop/proc_snap/overview-of-process-snapshotting
// PssCaptureSnapshot docs in https://learn.microsoft.com/en-us/windows/win32/api/processsnapshot/nf-processsnapshot-psscapturesnapshot
// PssQuerySnapshot docs in https://learn.microsoft.com/en-us/windows/win32/api/processsnapshot/nf-processsnapshot-pssquerysnapshot
// PssFreeSnapshot docs in https://learn.microsoft.com/en-us/windows/win32/api/processsnapshot/nf-processsnapshot-pssfreesnapshot

// Use golang.org/x/sys/windows/mkwinsyscall instead of adriansr/mksyscall
// below once https://github.com/golang/go/issues/42373 is fixed.
Expand All @@ -29,6 +30,7 @@ package process

//sys PssCaptureSnapshot(processHandle syscall.Handle, captureFlags PSSCaptureFlags, threadContextFlags uint32, snapshotHandle *syscall.Handle) (err error) [failretval!=0] = kernel32.PssCaptureSnapshot
//sys PssQuerySnapshot(snapshotHandle syscall.Handle, informationClass uint32, buffer *PssThreadInformation, bufferLength uint32) (err error) [failretval!=0] = kernel32.PssQuerySnapshot
//sys PssFreeSnapshot(processHandle syscall.Handle, snapshotHandle syscall.Handle) (err error) [failretval!=0] = kernel32.PssFreeSnapshot

// The following constants are PssQueryInformationClass as defined on
// https://learn.microsoft.com/en-us/windows/win32/api/processsnapshot/ne-processsnapshot-pss_query_information_class
Expand Down
9 changes: 9 additions & 0 deletions metric/system/process/zsyscall_windows.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.