Skip to content

Fix getOpenFileDescriptors() to use _Total PDH counter instance#3158

Merged
dbwiddis merged 4 commits intooshi:masterfrom
rohan-coder02:fix/pdh-open-file-descriptors-total
Apr 16, 2026
Merged

Fix getOpenFileDescriptors() to use _Total PDH counter instance#3158
dbwiddis merged 4 commits intooshi:masterfrom
rohan-coder02:fix/pdh-open-file-descriptors-total

Conversation

@rohan-coder02
Copy link
Copy Markdown
Collaborator

@rohan-coder02 rohan-coder02 commented Apr 14, 2026

Fix PdhUtilFFM.getOpenFileDescriptors() to query the pre-aggregated _Total PDH counter instance instead of wildcard.

Aligns FFM implementation with the working JNA version for better efficiency.

Removes loop iteration and directly returns the single handle count value.

Summary by CodeRabbit

  • Refactor

    • Improved Windows handle-count retrieval by switching to a direct aggregate counter query, simplifying logic and reducing overhead.
    • No public API signatures changed.
    • Existing error behavior (returning an error indicator on failure) is preserved.
  • Fix

    • When merging filesystem entries on Windows, prefer the name from system/WMI data so store names are more accurate.
  • Chore

    • Internal native binding adjustments to support the new retrieval approach.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3c410fc4-0905-4163-a44b-151118e65643

📥 Commits

Reviewing files that changed from the base of the PR and between cd350b3 and 9a0746d.

📒 Files selected for processing (1)
  • oshi-core-java25/src/main/java/oshi/software/os/windows/WindowsFileSystemFFM.java
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Test JDK 21, windows-latest
  • GitHub Check: Test JDK 25, windows-latest (+ JNA==FFM)
  • GitHub Check: Test JDK 21, macos-latest
  • GitHub Check: Test JDK 11, macos-latest
  • GitHub Check: Test JDK 25, macos-latest (+ JNA==FFM)
  • GitHub Check: Test JDK 11, ubuntu-latest
  • GitHub Check: Lint (Spotless, Checkstyle, ForbiddenAPIs, Javadoc)
  • GitHub Check: Analyze (java)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dbwiddis
Repo: oshi/oshi PR: 3014
File: oshi-core-java25/src/main/java/oshi/util/platform/windows/PdhUtilFFM.java:87-89
Timestamp: 2026-03-29T20:03:00.830Z
Learning: In the OSHI FFM codebase (oshi-core-java25), all PDH/Kernel32/COM wrapper methods in classes like PdhFFM, Kernel32FFM, etc. declare `throws Throwable` solely as a compiler artifact of `MethodHandle.invokeExact`. They are native calls and do not throw Java checked exceptions in practice. The enclosing `getOpenFileDescriptors()` method in PdhUtilFFM already has an outer `catch (Throwable t)` that covers the entire method body including any `finally` blocks, so no exception-masking concern exists for cleanup calls like `PdhCloseQuery` in `finally`.
📚 Learning: 2026-04-03T02:07:47.621Z
Learnt from: dbwiddis
Repo: oshi/oshi PR: 3110
File: oshi-core-java25/src/main/java/oshi/hardware/platform/mac/MacHWDiskStoreFFM.java:119-121
Timestamp: 2026-04-03T02:07:47.621Z
Learning: In OSHI’s Java FFM/API implementation classes (e.g., *FFM.java) that implement/override `updateAttributes()`, preserve the intentional failure-handling pattern: catch `Throwable` and return `false` without logging. `updateAttributes()` is a public API whose contract is to signal failure via the boolean return value, and callers rely on that return value (so emitting logs from this path is not desired and should not be introduced during review unless the method contract changes).

Applied to files:

  • oshi-core-java25/src/main/java/oshi/software/os/windows/WindowsFileSystemFFM.java
🔇 Additional comments (1)
oshi-core-java25/src/main/java/oshi/software/os/windows/WindowsFileSystemFFM.java (1)

119-125: LGTM — aligns FFM merge behavior with JNA.

Using wmiVolume.getName() here matches the existing JNA pattern in oshi-core/src/main/java/oshi/software/os/windows/WindowsFileSystem.java (around line 122), so the two implementations now produce consistent file store names (e.g., "Local Fixed Disk (C:)"). Updated comment accurately reflects the new behavior.


📝 Walkthrough

Walkthrough

Replaced per-instance PDH enumeration in PdhUtilFFM.getOpenFileDescriptors() with a direct query of \\Process(_Total)\\Handle Count. Added a new foreign-function binding and public wrapper for PdhGetFormattedCounterValue in PdhFFM. Adjusted merge behavior to prefer WMI-derived name when merging volumes in WindowsFileSystemFFM.

Changes

Cohort / File(s) Summary
PDH Counter Query Simplification
oshi-core-java25/src/main/java/oshi/util/platform/windows/PdhUtilFFM.java
Removed per-instance enumeration and summation; now returns the first formatted counter value's largeValue (reads \\Process(_Total)\\Handle Count) and eliminated manual instance-name filtering and offset math.
FFM Binding Addition
oshi-core-java25/src/main/java/oshi/ffm/windows/PdhFFM.java
Added a private MethodHandle downcall for PdhGetFormattedCounterValue and a public wrapper PdhGetFormattedCounterValue(...); static import ordering adjusted.
WMI Name Merge Change
oshi-core-java25/src/main/java/oshi/software/os/windows/WindowsFileSystemFFM.java
When merging existing result and a WMI volume match, replace the local OSFileStore name with wmiVolume.getName() (use WMI-derived name instead of preserving local volume.getName()).

Sequence Diagram(s)

(Skipped)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Poem

I’m a rabbit with a tiny pad,
I hopped the loops and made code glad.
One total counter, neat and clear,
Less bustle now — the handles cheer. 🐇✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: updating getOpenFileDescriptors() to query the _Total PDH counter instance directly instead of iterating over instances. This aligns with the primary modification in PdhUtilFFM.java.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.06%. Comparing base (764bf16) to head (9a0746d).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...oshi/software/os/windows/WindowsFileSystemFFM.java 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3158      +/-   ##
==========================================
+ Coverage   57.84%   58.06%   +0.21%     
==========================================
  Files         324      324              
  Lines       17394    17375      -19     
  Branches     3069     3066       -3     
==========================================
+ Hits        10062    10088      +26     
+ Misses       5857     5804      -53     
- Partials     1475     1483       +8     
Flag Coverage Δ
linux ?
macos ?
macos-latest 29.83% <0.00%> (?)
ubuntu-latest 26.59% <0.00%> (?)
windows ?
windows-latest 32.02% <88.88%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbwiddis
Copy link
Copy Markdown
Member

This is still failing the JNA == FFM test because the actual counter is ProcessInformation not Process. That was originally linked in the JNA code by the class name; extracting all those enums into a separate class lost that context.

{rohan-sarnad} and others added 3 commits April 16, 2026 13:55
PdhGetFormattedCounterArray does not work with a specific instance
counter path like \\Process(_Total)\\Handle Count. Use
PdhGetFormattedCounterValue instead, which is the correct API for
single-instance counters.
@dbwiddis dbwiddis force-pushed the fix/pdh-open-file-descriptors-total branch from bba7d72 to cd350b3 Compare April 16, 2026 21:45
The FFM merge was preserving the local volume name from
GetVolumeInformation (e.g. "Windows (C:\\\)") instead of using the
WMI description (e.g. "Local Fixed Disk (C:)") like JNA does.
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