> 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
