bobpaulin commented on PR #11543: URL: https://github.com/apache/nifi/pull/11543#issuecomment-5318028448
Hi @markap14 > Two more concerns about the lifecycle of the temporary instance and the temporary ClassLoader, both inline. > > Separately, and more importantly than either of them: I do not believe any of the five new tests in `ClasspathVerifyIT` can fail without this change, so I am not able to confirm from the PR that the reported problem is actually resolved. You are correct. I believe I have the scenario properly captured now. A class that would be supplied from the JAR that is available on a method signature AND the JAR's path must be coming from a Parameter Value Reference on the Component. I've removed the other tests that just provide superficial confirmation of the ability to use classes from the JAR via reflection as this is not relevant to the actual issue. > > For `testConnectorMethodLoadsClassFromCommittedClasspath`, the only test that exercises `invokeConnectorMethod` on a success path: `VerifyClasspathResource.CLASSPATH_RESOURCE` has no default value, so `isClasspathDifferent` is correctly `false` and the branch is decided entirely by `isReloadAdditionalResourcesNecessary()`. That leaves two possibilities, and both pass on `main`: > > 1. If the connector's flow update sets the property on the existing processor, `AbstractComponentNode.setProperties` calls `reload(...)`, and `StandardProcessorNode.reload` sets `additionalResourcesFingerprint` using the same value lookup that `isReloadAdditionalResourcesNecessary()` uses. The fingerprints match, the new branch is skipped, and the test runs the pre-existing live-component path -- where the live ClassLoader has just been reloaded with the asset JAR, so `Class.forName` succeeds. > 2. If instead flow synchronization creates the processor fresh, the fingerprint is null and the new branch is taken, but `StandardFlowManager` built the live component with `addClasspathUrls(additionalUrls)` in that case, so the live path would have succeeded as well. I think the introduction of the Parameter Value reference introduces a third path which is what is required for this bug to be reproduced. Hardcoding the path does not reproduce the bug and the tests pass with or without this change. > > The remaining four tests exercise `ProcessorFacade.verify`, which this PR does not change, or only assert that a bogus class name yields a `FAILED` outcome, which holds either way. Agreed these are now removed. > > The gap looks structural rather than an oversight. Verification can be driven into the interesting state because the caller supplies property overrides; connector method invocation has no override mechanism, so I could not find a state reachable through the connector API where the live component's ClassLoader lacks resources that the `ProcessContext` can resolve. Could you add a test that fails on `main` in the expected way -- `ClassNotFoundException` or `NoClassDefFoundError` out of the connector method because the live component's ClassLoader lacks the resource -- and share the scenario that produces it? See above. I think the scenario is reproducing properly now. And the fix provided addresses the issue. I've changed the fix to only involve the temp components in the WORKING context which I believe should address some of the stateful concerns about execution the the ACTIVE context. If we're running on an ACTIVE context all the classloading concerns should have already been addressed so there's no need for temp components on that path. Thank you again for the feedback I think this fix better addresses the concerns you've raised let me know if there are still items that you feel are not addressed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
