RFR: 4200096: OffScreenImageSource.removeConsumer NullPointerException [v3]
Phil Race
prr at openjdk.org
Wed Apr 12 04:45:35 UTC 2023
On Wed, 12 Apr 2023 04:00:57 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 one additional commit since the last revision:
>
> 4200096: rewrite solution, add LegitimateNullPointerTest
>
> Also I'm breaking out the unit tests (now there are 4 of them) into their own directory.
>
> The new LegitimateNullPointerTest confirms that we DO want to see NullPointerExceptions printed to System.err IF they come from the ImageConsumer itself.
>
> In this ticket most of our focus has been on the NPE's that stem from removing an ImageConsumer from OSIS mid-production (so the NPE was when OSIS tried to interact with its `theConsumer` field). This test tries to add other possible NPE's to our consideration.
>
> This new test passed in the master branch before this branch. I want to preserve this existing behavior if this proposal is accepted.
> 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.
Yes, that's a good point. Just check if consumer is null and if it isn't, then the NPE can't be from de-referemcing
that and should be reported.
Also note my suggestion from yesterday that the NPE could still be reported if a system property is set.
If you prefer to have more discussion to agreement/clarification before coding up something just to find we revise it again , or if you prefer to code it up and ask "what do you think about this ? ", so there's clarity on what you mean, either is fine.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13408#issuecomment-1504605358
More information about the client-libs-dev
mailing list