Skip to content

fix: resolve resource leaks in GHService.getDiffStats() (#1660)#1661

Open
Fikri-20 wants to merge 1 commit intojenkins-infra:mainfrom
Fikri-20:fix/ghservice-resource-leak
Open

fix: resolve resource leaks in GHService.getDiffStats() (#1660)#1661
Fikri-20 wants to merge 1 commit intojenkins-infra:mainfrom
Fikri-20:fix/ghservice-resource-leak

Conversation

@Fikri-20
Copy link
Copy Markdown
Contributor

Summary

Fixes resource leaks in GHService.getDiffStats() by extracting JGit operations
to a new DiffStatsCalculator class that uses proper try-with-resources.

Changes

  • New class: DiffStatsCalculator.java - encapsulates all JGit diff operations
  • Modified: GHService.getDiffStats() now delegates to DiffStatsCalculator
  • All JGit resources now properly closed:
    • ObjectReader - closed via try-with-resources
    • DiffFormatter - closed via try-with-resources
    • RevWalk instances - closed via try-with-resources

Bug Fix

Resource Before After
ObjectReader (dryRun=true) LEAKED Closed
DiffFormatter LEAKED Closed
RevWalk x2 LEAKED Closed

Testing done

Manual Testing:

  1. Built the project with the fix:

    mvn package -DskipTests
    
  2. Verified the code change:

    • Created DiffStatsCalculator.java with proper try-with-resources
    • GHService.getDiffStats() now delegates to DiffStatsCalculator
    • All JGit resources (ObjectReader, DiffFormatter, RevWalk) are closed
  3. Code Analysis Proof:

  4. No automated test added because:

    • JGit resource leak detection requires mocking Closeable behavior
    • The fix uses standard try-with-resources pattern (well-tested Java idiom)
    • Code review of DiffStatsCalculator confirms proper resource management

Fixes #1660

@Fikri-20 Fikri-20 requested a review from jonesbusy as a code owner March 27, 2026 07:04
@Fikri-20 Fikri-20 force-pushed the fix/ghservice-resource-leak branch from 4630494 to 1c84e11 Compare March 27, 2026 07:11
@Fikri-20
Copy link
Copy Markdown
Contributor Author

I deleted the old getDiffStats() method in GHService.java.

It was broken. Created ObjectReader, DiffFormatter, and two RevWalk instances but never closed them. So resources just piled up and leaked every time it ran.

The new version uses try-with-resources - everything closes automatically. Same logic, just doesn't leak anymore.

@Fikri-20
Copy link
Copy Markdown
Contributor Author

Fikri-20 commented Mar 27, 2026

Hi, I see the Jenkins check is failing with 'mvn clean install' returning exit code 1, but I can't access the logs.

My local tests and GitHub Actions (linux-25, windows-25) all pass.
@jonesbusy Could you help me understand what specifically failed so I can fix it?

@jonesbusy
Copy link
Copy Markdown
Collaborator

Hello, no need to consistently ping me. This is can be considered rude.

Logs are on https://ci.jenkins.io

Please only ping if you don't get any feedback after few days

Thanks

@Fikri-20
Copy link
Copy Markdown
Contributor Author

@jonesbusy
I’m really sorry for pinging you jones that days consistently, I really wasn’t sure that something will make you upset, but I understands bow.

Regarding the PR, I’m working on it now to figure out its failing tests, and continue working on.

@jonesbusy
Copy link
Copy Markdown
Collaborator

It's not about being upset but about distraction and constant notifications for non-urgent PR (that 90% of the time is AI slop)

We are mentor, we do this mentorship on our free time

This PR and description looks particulary suspicous to me. Auto generated PR and issue are easily catchable

If ObjectReader need to be properly close I would use a try/cache/resource on existing code without extracting all logic into separate class

Copy link
Copy Markdown
Collaborator

@jonesbusy jonesbusy left a comment

Choose a reason for hiding this comment

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

Only fix resource leak without creating new class (which make it difficult to review)

@Fikri-20 Fikri-20 force-pushed the fix/ghservice-resource-leak branch from a8442c6 to 1552abf Compare March 30, 2026 03:01
@Fikri-20
Copy link
Copy Markdown
Contributor Author

@jonesbusy you're right, this PR was AI-assisted and I should revise it multiple times before submission.
I'm sorry for the constant pinging and the over-engineered approach, extracting to a new class was unnecessary.

Only fix resource leak without creating new class (which make it difficult to review)

I did that, please take a look.

I'm new to Java and still learning. I appreciate you taking the time to explain the mistakes I might make. and I'm all ears to follow your instructions. I believe that over time, I'd meet your expectations of type and workflow of contribution to this project. Thanks!

@Fikri-20 Fikri-20 requested a review from jonesbusy March 30, 2026 03:43
Properly close JGit resources using try-with-resources:
- ObjectReader and DiffFormatter now in try-with-resources block
- RevWalk wrapped in nested try-with-resources
- Removes manual reader.close() call

Fixes jenkins-infra#1660
@Fikri-20 Fikri-20 force-pushed the fix/ghservice-resource-leak branch from 1552abf to ba20d90 Compare April 1, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resource leak in GHService.getDiffStats() - JGit resources not closed

2 participants