RFR: 4200096: OffScreenImageSource.removeConsumer NullPointerException [v2]

[Phil Race]

On Tue, 11 Apr 2023 03:49:25 GMT, Jeremy <duke at openjdk.org> wrote:

>> This resolves a 25 year old P4 ticket: a NullPointerException is printed to System.err needlessly.
>> 
>> This resolution involves confirming that an ImageConsumer is still registered before every notification.
>> 
>> I'll understand if this is rejected as unimportant, but I stumbled across this in the real world the other day and thought this was a simple enough bug to practice on.
>
> Jeremy has updated the pull request incrementally with six additional commits since the last revision:
> 
>  - 4200096: OffScreenImageSource.removeConsumer NullPointerException
>    
>    Make sure OffScreenImageSource#addConsumer doesn't throw a NPE if the argument is null.
>    
>    The rationale here is:
>    The preexisting implementation wouldn't throw a NPE, so we shouldn't change that now.
>    
>    (Or more specifically: the preexisting implementation *would* throw a NPE, but it would also catch it and print it to System.err. The caller wouldn't need to anticipate a NPE.)
>  - 4200096: OffScreenImageSource.removeConsumer NullPointerException
>    
>    Changing `runImageConsumerTest` to make sure we're explicitly testing a `OffScreenImageSource`.
>    
>    By contrast: the `runImageDimensionTest` is more of an integration-style test that *happens* to test the `OffScreenImageSource`.
>  - 4200096: OffScreenImageSource.removeConsumer NullPointerException
>    
>    Adding createAbstractImage() with a little documentation to clarify why we're creating an image the way we are.
>  - 4200096: OffScreenImageSource.removeConsumer NullPointerException
>    
>    Re-adding the 'synchronized' keyword.
>    
>    We can probably do without it now, but I'm just trying to minimize possible unintended side-effects of this PR. Nobody asked for improved multithreaded support. And this class is used to iterate over BufferedImages (which are already in memory), so (as long as the ImageConsumer isn't taking too long -- or blocking) this should be very fast.
>  - 4200096: OffScreenImageSource.removeConsumer NullPointerException
>    
>    In code review prrace pointed out:
>    > I'd capture "theConsumer" into a local var in each of the methods sendPixel() and produce() and check and use the local var
>    
>    https://github.com/openjdk/jdk/pull/13408#issuecomment-1502348061
>    
>    This is actually my previous/original draft for JDK-4200096 ( 49a49ee8585e8a4b5f33a6af04f2ce0165bf1199 ), but I shied way from this approach because I was afraid code reviewers would point out its changes are excessively invasive for the original complaint. (For ex: now OSIS supports multithreading, which nobody asked for. Should I re-add the `synchronized` method modifiers? (although they are theoretically no longer necessary, keeping them reduces the risk of unintended consequences...) Or if multithreaded support is worth keeping: should I at least add a unit test for it? These questions seemed like they're straying off-topic from the original bug.)
>  - 4200096: OffScreenImageSource.removeConsumer NullPointerException
>    
>    In code review mrserb pointed out we shouldn't use System.exit(1) as an error condition.
>    
>    https://github.com/openjdk/jdk/pull/13408#discussion_r1162182212

I think I am starting to get the overall picture here.

The fix I cited (4905411) was essentially a fix for 4200096, since it prevented NPEs breaking the app
and was how they chose to avoid the constant null checking. Just catch the NPE and return.
The reasons for the printStackTrace() are not entirely clear. You'd know there something
removed the consumer but not where. FYI I added calls to dumpStack and ran your app
and the test program from 4905411 : java/awt/image/OffScreenImageTest/CropFilterTest.java
and the removal of the consumer can happen in multiple places for multiple reasons.
I can see why "handling" the null was more straightforward than hunting those down
and doing serious re-working of the code.

So
- Perhaps all you really needed to do here was remove the printStackTrace, catching the NPE
is a valid way to handle this if you want to bail as soon as the consumer is removed

- The catch block means that anyone adding new callbacks in here doesn't have to also
   add yet another check. The difference maker would be if ending up with an NPE is really
   common here, and I don't know if it is. If you keep all the null checks then I think
   several in-line comments are warranted.

- I'm back peddling  on continuing with the captured variable. Its a behavioural change.

- I don't understand the reason you are now allowing multiple consumers, nor what
   the consequences would be.

- I note that you are now not allowing someone to add a null consumer.
   I'd wondered why there was no upfront check
   I think this is OK since it looks completely compatible - the null would have been
   immediately de-referenced and the exception thrown.

- I  would definitely keep the synchronized modifiers

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13408#issuecomment-1504137827