search

eclipse-sisu / sisu-project

Sisu Inject
https://eclipse.dev/sisu/
Eclipse Public License 2.0
18 stars 15 forks source link

Do not silently fail in SpaceScanner when classes could not be parsed #97

Closed kwin closed 8 months ago

kwin commented 11 months ago

Currently issues with parsing class files are only issued on DEBUG level (https://github.com/eclipse/sisu.inject/blob/a9752361662edac045f16129a49e1a8b625aa604/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SpaceScanner.java#L115). As this is a frequent cause of trouble (https://lists.apache.org/thread/6wxrhlkrqsl7wgr730xd50h55t7ff0vy) this should at least optionally lead to an exception or at least log entry with ERROR level.

kwin commented 11 months ago

@cstamas Do you remember edge cases where an exception here should be just discarded?

cstamas commented 11 months ago

I'd make this configurable, and let Sisu behave as "today" (log only on DEBUG), but Maven could "flip it" and have fail?

cstamas commented 11 months ago

I'd make this configurable, and let Sisu behave as "today" (log only on DEBUG), but Maven could "flip it" and have fail?

I don't, maybe @mcculls ?

kwin commented 11 months ago

Also the dedicated exception handler in https://github.com/eclipse/sisu.inject/blob/a9752361662edac045f16129a49e1a8b625aa604/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/space/SpaceScanner.java#L111 seems to be no longer relevant, as more recent ICU4J versions fixed their class files (https://bugs.eclipse.org/bugs/show_bug.cgi?id=197733#c6)

mcculls commented 11 months ago

as more recent ICU4J versions fixed their class files

Projects may well have older ICU4J versions on their class-path, and removing this catch would break them - either at build-time (if generating the index) or at runtime if they rely on runtime scanning for components.

Since this particular catch block is not the source of any issues, I'd leave it in place rather than risk breaking builds.

mcculls commented 11 months ago

@cstamas Do you remember edge cases where an exception here should be just discarded?

Originally it was to protect against a rogue resource from breaking startup (ie IOException) when doing runtime scanning - I don't remember the exact details, but I do remember that the resource in question wasn't something of interest so it was safe to skip. It also wasn't straightforward to avoid the resource issue, because it happened when Sisu was being deployed onto a particular web-app not under our control. Over various iterations we widened the catch to handle other reported conditions - in all cases it was related to resources we were ok skipping at runtime.

Nowadays we lean more on the index at runtime, so it would make sense to be able to put the ClassSpaceScanner into strict mode when generating the index. However I would prefer for the default to remain as non-strict for compatibility reasons, especially when doing runtime scanning.

We can always change the index generator (both the annotation processor and the maven plugin) to then be strict.

I would also like to keep the ICU4J-related catch block to avoid breaking older builds/deployments.

kwin commented 11 months ago

I would also like to keep the ICU4J-related catch block to avoid breaking older builds/deployments.

There is IMHO no need for two catch blocks. A warning in lenient mode for any kind of exception does not modify functionality and still allows to use ancient ICU4J (this is even tested in QualifiedScanningTest.testBrokenScanning which still works in lenient mode) and correctly fails in strict mode. Not logging (in lenient mode)/not throwing (in strict mode) for exception ArrayIndexOutOfBoundsException might otherwise easily lead to false negatives.

kriegaex commented 8 months ago

Please, if you decide to close #108 as a duplicate, note that I have a concrete example there which leads to problems, see also https://github.com/codehaus-plexus/plexus-compiler/issues/347, especially https://github.com/codehaus-plexus/plexus-compiler/issues/347#issuecomment-1886587188.

The affected plugin is Maven Compiler when using certain Plexus compilers. The problems are caused by Sisu loading scanned classes on top of ASM being able to actually correctly parse them. But also in the case where scanning them with ASM would fail already due to the byte code being newer than the embedded ASM understands, optionally it should be possible to propagate an exception to the caller, instead of just catching and logging it without the caller noticing.

kwin commented 8 months ago

Please check the related PR at https://github.com/eclipse/sisu.inject/pull/98 if that solves your concern.

kriegaex commented 8 months ago

LGTM after just taking a look at the code. Neither am I a committer in Maven Compiler nor in Plexus Compiler, but I hope they will pull in this change after there is a release containing it.