Nullability fixes for AtomicReference #35514
Conversation
Signed-off-by: Manu Sridharan <[email protected]>
msridhar
force-pushed
the
atomicreference-nullable-fixes
branch
from
38b84f0 to
7a39a3a
Compare
spring-projects-issues
added
the
status: waiting-for-triage
|
FYI @sdeleuze |
| .send(FlowAdapters.toPublisher(new OutputStreamPublisher<>( | ||
| os -> body.writeTo(StreamUtils.nonClosing(os)), new ByteBufMapper(outbound), | ||
| executorRef.getAndSet(null), null))); | ||
| Objects.requireNonNull(executorRef.getAndSet(null)), null))); |
There was a problem hiding this comment.
This is the only logic change in this PR (all other changes are just adding @Nullable annotations). I'm pretty confident in the change based on the surrounding code but I'm not an expert so it should be reviewed
There was a problem hiding this comment.
LGTM, but I would welcome a confirmation from @jhoeller and/or @rstoyanchev.
There was a problem hiding this comment.
After discussing with Juergen and Rossen, looks fine but we would like @violetagg confirmation.
There was a problem hiding this comment.
There is already assert in OutputStreamPublisher, so do we need this here or it is to suppress the warnings?
basically we will check twice.
There was a problem hiding this comment.
Good point. That's not a hot code path but conceptually this double check is annoying.
@msridhar Would declaring a AtomicReference<@NullUnmarked Executor> executorRef field be acceptable here with your upcoming changes to not trigger warnings without having to do the double check?
sdeleuze
added
type: enhancement
sdeleuze
added
the
status: waiting-for-internal-feedback
Make the contents of some
AtomicReferencevariables@Nullable. This is in preparation for uber/NullAway#1298 which causes new errors to be reported for these variables.