Add NullAway to javaagent-tooling and javaagent#17719
Conversation
…nt modules Part of open-telemetry#15991 Enables NullAway for javaagent-tooling, javaagent-tooling-java9, and javaagent modules, and adds required @nullable annotations to AgentInstaller, AgentStarterImpl, DefineClassHandler, ExtensionClassLoader, InputStreamUrlConnection, OpenTelemetryInstaller, RemappingUrlConnection, SafeServiceLoader, Utils, ConfigurationFile, JavaagentDistributionAccessCustomizerProvider, FieldBackedImplementationInstaller, VirtualFieldImplementationsGenerator, InstrumentationLoader, InstrumentationModuleInstaller, MuzzleMatcher, RegexUrlTemplateCustomizerInitializer, IndyBootstrap, InstrumentationModuleClassLoader, ClassLoaderMap, ClassLoaderValue, Trie, and TrieImpl. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
- AgentStarterImpl: add @nullable to two ClassFileTransformer.transform methods that can return null, matching the existing pattern on the sibling LaunchHelperClassFileTransformer. - HelperInjector: guard null helpersSource with requireNonNull at the call into HelperClassDefinition.create, which delegates to BytecodeWithUrl.create (rejects null with IllegalArgumentException). The message documents the runtime contract: null helpersSource is only valid when helperClassNames is empty. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
zeitlinger
force-pushed
the
nullaway-tooling
branch
from
fa3eb1a to
9c0e6c5
Compare
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
AI review below (see #17889)
NullAway rollout for javaagent-tooling / javaagent — mostly @Nullable annotations. A few spots worth a second look:
DefineClassHandler: switching fromnull-return + null-check to a sharedNOPsentinel, combined with the newexit()fallback, can overwrite an outer context's ThreadLocal entry when a nested J9-cookie class definition occurs.OpenTelemetryInstaller: extensions and listeners previously sawnullConfigPropertiesin declarative mode; they now see an empty-map-backed instance — please confirm this is intentional.- A couple of minor style nits (redundant
requireNonNull, per-elementrequireNonNullinside a stream).
| @Override | ||
| public void exit() { | ||
| defineClassContext.set(previous); | ||
| defineClassContext.set(previous != null ? previous : NOP); |
There was a problem hiding this comment.
[General] Setting the ThreadLocal to NOP here can silently corrupt outer state.
beforeDefineClass now returns DefineClassContextImpl.NOP for the J9ROMCLASSCOOKIE path without calling enter() (so the ThreadLocal is not pushed). But afterDefineClass has been simplified to always call context.exit(), so NOP.exit() runs — and since NOP.previous is null, this line writes NOP into defineClassContext.
In a nested scenario (outer beforeDefineClass triggers a nested class definition via Class.forName during super-type resolution, and the nested class hits the J9 path), the inner NOP.exit() overwrites the outer context with NOP. Between that point and the outer afterDefineClass, isFailedClass() / getSuperTypes() will read NOP instead of the outer context.
Consider making exit() a no-op on NOP (e.g. early-return when this == NOP), or restore the previous behavior of skipping exit() when beforeDefineClass returned the no-op sentinel.
| AgentDistributionConfig.set(AgentDistributionConfig.fromConfigProperties(configProperties)); | ||
| } else { | ||
| // Declarative config path: no ConfigProperties available, use empty defaults | ||
| configProperties = DefaultConfigProperties.createFromMap(emptyMap()); |
There was a problem hiding this comment.
[General] Behavior change worth confirming: previously, in declarative-config mode, the returned AutoConfiguredOpenTelemetrySdk.getConfig() was null. Downstream callers (AgentInstaller.setBootstrapPackages, configureIgnoredTypes, BeforeAgentListener#beforeAgent, AgentExtension#extend) now receive an empty-map-backed ConfigProperties instead.
Any extension that disambiguated declarative mode via config == null will silently switch to the empty-config branch. Is this the intended contract going forward? If so, it's probably worth noting in the changelog / extension docs.
- InstrumentationLoader: inline getInstrumentation() into the InstrumentationModuleInstaller constructor; the constructor already calls requireNonNull on the parameter, so the local variable and redundant requireNonNull can go. - HelperInjector: hoist requireNonNull(helpersSource, ...) out of the stream map so it runs at most once, guarded by !helperClassNames.isEmpty() to preserve the "non-empty" semantics implied by the message. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
… frame beforeDefineClass returns NOP on the J9ROMCLASSCOOKIE path without calling enter() (no frame pushed). afterDefineClass always calls context.exit(), so NOP.exit() would reset the ThreadLocal — clobbering an outer frame still active on the stack during nested defineClass invocations. Override exit() on NOP as a no-op so the paired enter/exit invariant holds. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
|
I'm going to convert this PR to draft (just housekeeping), once you get CI passing, please mark it ready for review, thanks! |
2 participants
Blocked by #18090
Part of #15991
Enables NullAway for
javaagent-tooling,javaagent-tooling-java9, andjavaagentmodules and adds required@Nullableannotations. This is the largest module in the set.Files changed:
javaagent-tooling/build.gradle.kts,javaagent-tooling-java9/build.gradle.kts,javaagent/build.gradle.kts— enableotel.nullaway-conventionsAgentInstaller.java— userequireNonNullonAutoConfigureUtil.getConfig(); simplify config-null checkAgentStarterImpl.java—@NullableonextensionClassLoaderfield andgetExtensionClassLoader()returnDefineClassHandler.java—@NullableannotationsExtensionClassLoader.java—@NullableannotationsInputStreamUrlConnection.java—@NullableannotationsOpenTelemetryInstaller.java—@NullableannotationsRemappingUrlConnection.java—@NullableannotationsSafeServiceLoader.java—@NullableannotationsUtils.java—@NullableannotationsConfigurationFile.java—@NullableannotationsJavaagentDistributionAccessCustomizerProvider.java—@NullableannotationsFieldBackedImplementationInstaller.java—@NullableannotationsVirtualFieldImplementationsGenerator.java—@NullableannotationsInstrumentationLoader.java—@NullableannotationsInstrumentationModuleInstaller.java—@NullableannotationsMuzzleMatcher.java—@NullableannotationsRegexUrlTemplateCustomizerInitializer.java—@NullableannotationsIndyBootstrap.java—@NullableannotationsInstrumentationModuleClassLoader.java—@NullableannotationsClassLoaderMap.java,ClassLoaderValue.java,Trie.java,TrieImpl.java—@NullableannotationsDepends on: #17718 (nullaway-muzzle) should merge first so NullAway on tooling sees correct annotations on muzzle's public API