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

tweak(core): use si for process data #1027

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

million1156
Copy link

This PR is still a draft. I am not sure if exec is the best possible implementation, but it seems okay enough to create a draft PR.

This PR fixes an issue that was introduced with Windows 11 & Windows Server 2025, which removed wmic.

Instead of using the deprecated command, we instead call Get-WmiObject, whose documentation can be found here. The command we use to get the data is:

Get-WmiObject -Query "SELECT * FROM Win32_Process WHERE ProcessId = ${fxsPid}" |  Select-Object -ExpandProperty WorkingSetSize

, where:

  • fxsPid is the fxServer process
  • WorkingSetSize is the amount of memory fxServer is currently occupying.

Essentially, we're given the amount of memory (in bytes) that the process is currently consuming. So, we do the same division that we used to use for Linux processes.

The only concern I have here is that it may not be the best idea to use exec. However, I've tried my best to prevent any security issues. I'm curious if there's any better ways to go about this! But it seems like the most viable solution as of right now.

@million1156 million1156 changed the base branch from master to develop January 23, 2025 14:33
@tabarra
Copy link
Owner

tabarra commented Jan 23, 2025

Hey @million1156, thanks for the (draft) PR.

txAdmin uses a bunch of different ways to get a bunch of different host data, like windows / distro name, processor speeds, processes memory usage, etc. And all of that was built individually without much consideration for the overall.

More recently I've thinking of just using thinking of refactoring those parts of code to use the systeminformation library.
In the past I had avoided it because in some systems, even getting the processes memory usage could cost about 10 seconds of the thread going 100% usage, but now I think it's probably the time for the change.

Here is a copypaste from the devnotes about this subject:

@million1156
Copy link
Author

million1156 commented Jan 23, 2025

Hey @tabarra,

Thanks for the quick response! I've created a new commit that now uses si. It's a little more convoluted to get working, but it works in my testing :) Let me know what you think!

EDIT: In the future I want to try and replace every stats, but I think this is a great start since this currently affects anyone who's using Windows Server 2025 or Windows 11 (i.e. a dev hosting a server from their home computer/server)

@million1156 million1156 changed the title tweak(core): use get-wmiobject for process data tweak(core): use si for process data Jan 23, 2025
@million1156 million1156 marked this pull request as ready for review January 23, 2025 15:34
@million1156 million1156 requested a review from tabarra as a code owner January 23, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants