On Tue, 11 Apr 2023 03:49:25 GMT, Jeremy <d...@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
