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

[MARTIFACT-75] Don't print stack trace from correctly functioning code #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Oct 20, 2024

No description provided.

@elharo elharo requested a review from michael-o October 20, 2024 21:16
@hboutemy
Copy link
Member

this does not really solve the issue, but just makes it less visible:

[WARNING] Corrupt jar file /Users/hboutemy/dev/maven/plugins/tools/maven-help-plugin/target/reference/org.apache.maven.plugins/maven-help-plugin-3.5.0.pom
zip END header not found

as fixed in #67, trying to open a .pom file as a jar is just a bug, not corrupted zip

when an issue is found openeing archive, having the full stack trace is IMHO useful to differentiate between bug like currently vs reqally corrupted zip (like nobody ever reported)

@elharo
Copy link
Contributor Author

elharo commented Oct 21, 2024

I agree this doesn't solve the issue. However, it does make it more visible. The real problem in this case is,as you noticed, that we're trying to open a pom as a jar. However, this log message now makes that a little more apparent since the problem is not obscured by an extra page of stack dump.

And in the more likely case where the jar file actually is corrupt (which does happen) this will also be clearer.

@hboutemy
Copy link
Member

hboutemy commented Oct 21, 2024

sorry, I don't find hiding the stacktrace improving visibility, nor putting original message on a newline: probably matter of taste
Differentiating ZipException from generic IOException is a nice idea, to differentiate file reading fro corrupted jar

FYI, I closed MARTIFACT-75 based on #67 = original code was not working properly (regression introduced in MARTIFACT-62): don't hesitate to create another Jira issue for that corrupted jar display

} catch (IOException e) {
log.warn("unable to open jar file " + file, e);
log.warn("unable to read jar file " + file + "\n" + e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

thinking again at it: if we do that, we need to display the exception class
like if the exception message was the first line of the stacktrace, as it brings useful info = the type of issue

with that pattern, ok, the rest of the stacktrace can be ignored

@michael-o michael-o removed their request for review October 22, 2024 07:12
@michael-o
Copy link
Member

I wouldn't suppress exceptions

@elharo
Copy link
Contributor Author

elharo commented Oct 22, 2024

MARTIFACT-75 is a very interesting and uncommon exception pattern. If I'm following the discussion correctly, there is in fact a bug in the maven-dependency-plugin code that causes it to load the wrong file. That leads to this zip exception. So yes, in this one case the stacktrace does help.

However that's the zebra case. The horses cases (i.e. when you hear hoof beats think horses, not zebras) is that the file is in fact corrupt. I have seen corrupt jar files in Maven repos, central included, many, many times over the years, though more frequently when I was in the business of deep scanning large dependency trees in groups of many projects. Back when I was doing that, it was pretty routine that somewhere in a scan of thousands of artifacts, you'd hit one or two corrupt jars.

Stack traces should be reserved for program bugs, usually indicated by runtime exceptions. In this very specific case, it turns out the ZipException actually is a program bug so a stack trace is helpful, but the vast majority of the time a ZipException or IOException indicates a problem in external data and not in the program itself. In that far more common case, the stack trace is unactionable noise.

@slachiewicz slachiewicz closed this Mar 1, 2025
@slachiewicz slachiewicz deleted the MARTIFACT-75 branch March 1, 2025 19:50
@elharo elharo restored the MARTIFACT-75 branch March 1, 2025 20:24
@elharo elharo reopened this Mar 1, 2025
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.

4 participants