RFR: 4200096: OffScreenImageSource.removeConsumer NullPointerException [v2]

[Jeremy]

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 don't like the idea of removing `e.printStackTrace()`. For every case we've discussed so far: I agree that sounds good/harmless. But if the ImageConsumer is buggy and ends up throwing its own NullPointerException: then I'd argue developers *need* that NPE printed to the console so they can see what's going on.

I'll add a new (4th) unit test for this condition: I DO expect a NPE from the ImageConsumer to be printed to System.err.

I think what I'm hearing (please correct me if I'm wrong) is: constantly confirming the ImageConsumer is still attached to the OffScreenImageSource hurts readability. So I'll propose a new approach for your consideration shortly.

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

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