Skip to content

chore: DefaultUpdateCheckManager Fix variable 'lock' is never used (PMD: UnusedPrivateField) #2376

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: DefaultUpdateCheckManager Fix variable 'lock' is never used (PMD: UnusedPrivateField) #2376

wants to merge 1 commit into from

Conversation

Pankraz76
Copy link
Contributor

No description provided.

@Pankraz76 Pankraz76 mentioned this pull request May 22, 2025
Closed
9 tasks
@Pankraz76 Pankraz76 force-pushed the fix-Variable-'lock'-is-never-used- branch from 2fb927a to 0999cbd Compare May 22, 2025 09:03
@Pankraz76 Pankraz76 marked this pull request as ready for review May 22, 2025 09:03
elharo
elharo requested changes May 22, 2025
View reviewed changes
...maven-compat/src/main/java/org/apache/maven/repository/legacy/DefaultUpdateCheckManager.java
@@ -303,7 +301,7 @@ private Properties read(File touchfile) {
Properties props = new Properties();

try (FileInputStream in = new FileInputStream(touchfile)) {
try (FileLock lock = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
try (FileLock ignored = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not ignored. Is PMD simply wrong here?

Copy link
Contributor Author

@Pankraz76 Pankraz76 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

really? check out the usage. It seems unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@desruisseaux point out correctly that its used indirectly, having the implizit close() call.

As IDEA and PMD both point this out is seems ok to rename to give proper context. Its ignored as handled by default.

Other way should be an inline comment for suppression.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used by the try with resources auto-close. See https://stackoverflow.com/questions/70773389/try-with-resource-without-declaring-variable

Unless you can remove the variable, PMD is wrong that this is unused. Maybe Idea is too.

Copy link
Contributor Author

@Pankraz76 Pankraz76 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

indeed. This is the only use, which is an exception of the normal pattern (using is explizit).

To make this clear the convention seems naming it ignored, at least considering IDEA and PMD, both recognize the variable as marked explizit, as only used implicit.

Now using the resource which the statement is all about, seems like an error, thats why its pointes out by these tools.
Im altho wondering why its not used, allowing a comment to explain too.
Then the comment end up in the code again, to be self explaining.

Pankraz76
Pankraz76 commented May 23, 2025
View reviewed changes
Copy link
Contributor Author

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

the block is super small, its defacto unused or im not Vincent Potucek.

...maven-compat/src/main/java/org/apache/maven/repository/legacy/DefaultUpdateCheckManager.java
@@ -303,7 +301,7 @@ private Properties read(File touchfile) {
Properties props = new Properties();

try (FileInputStream in = new FileInputStream(touchfile)) {
try (FileLock lock = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

the block is super small, its defacto unused.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its is an error indication, therefore should be named according to convention.

...maven-compat/src/main/java/org/apache/maven/repository/legacy/DefaultUpdateCheckManager.java
@@ -303,7 +301,7 @@ private Properties read(File touchfile) {
Properties props = new Properties();

try (FileInputStream in = new FileInputStream(touchfile)) {
try (FileLock lock = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
try (FileLock ignored = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

really? check out the usage. It seems unused.

@Pankraz76 Pankraz76 requested a review from elharo May 23, 2025 06:38
@Pankraz76 Pankraz76 force-pushed the fix-Variable-'lock'-is-never-used- branch from 0999cbd to 693590a Compare May 23, 2025 06:51
@Pankraz76 Pankraz76 requested a review from desruisseaux May 23, 2025 06:52
@desruisseaux
Copy link
Contributor

It is used implicitly in a implicit finally block, even if we don't see it in the code.

@Pankraz76 Pankraz76 mentioned this pull request May 23, 2025
Draft
@Pankraz76 Pankraz76 force-pushed the fix-Variable-'lock'-is-never-used- branch from 693590a to 2483ec9 Compare May 23, 2025 14:31
@Pankraz76
Copy link
Contributor Author

It is used implicitly in a implicit finally block, even if we don't see it in the code.

yes, then it should be named accordingly.

@Pankraz76
Copy link
Contributor Author

dont we have test for this lock? afaik the lock only gets closed but not like open or checked if its ready to go. So calling this should do the trick as well:
getProjectLock(session, mojoDescriptor).lock();

elharo
elharo requested changes May 23, 2025
View reviewed changes
...maven-compat/src/main/java/org/apache/maven/repository/legacy/DefaultUpdateCheckManager.java
@@ -303,7 +301,7 @@ private Properties read(File touchfile) {
Properties props = new Properties();

try (FileInputStream in = new FileInputStream(touchfile)) {
try (FileLock lock = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
try (FileLock ignored = in.getChannel().lock(0, Long.MAX_VALUE, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's used by the try with resources auto-close. See https://stackoverflow.com/questions/70773389/try-with-resource-without-declaring-variable

Unless you can remove the variable, PMD is wrong that this is unused. Maybe Idea is too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo requested changes

@desruisseaux desruisseaux Awaiting requested review from desruisseaux

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Pankraz76 @desruisseaux @elharo