RFR: 4200096: OffScreenImageSource.removeConsumer NullPointerException [v2]

Jeremy duke at openjdk.org
Tue Apr 11 03:49:25 UTC 2023


> 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

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/13408/files
  - new: https://git.openjdk.org/jdk/pull/13408/files/73e9f010..36feaad0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13408&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13408&range=00-01

  Stats: 90 lines in 2 files changed: 46 ins; 10 del; 34 mod
  Patch: https://git.openjdk.org/jdk/pull/13408.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13408/head:pull/13408

PR: https://git.openjdk.org/jdk/pull/13408



More information about the client-libs-dev mailing list