RFR: 8177136: Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger, and System.getLogger should throw IllegalCallerException if there is no caller on the stack.

[David Holmes]

On 21/03/2017 6:56 AM, Daniel Fuchs wrote:
> Hi Mandy
>
> On 20/03/17 20:40, Mandy Chung wrote:
>> Hi Daniel,
>>
>> Have you considered default to the unnamed module when there is no
>> caller frame on the stack?
>>
>> I don’t think we should make these APIs not callable from JNI attached
>> thread even though very rarely be called from JNI (not sure any report
>> on the current behavior throwing NPE).
>
> I don't think we should make assumption or guesses.
> As Alan stated: "If there's no caller then you can't make any
> assumptions as to who the caller is. Having these methods assume
> java.base or the unnamed module of some class loader doesn't seem right
> as there just isn't enough context."

I disagree. The "default" context could be the unnamed-module of the 
application classloader - which would be the normal initial context for 
a piece of Java code. If the JNI code needs something different to that 
then it can make arrangements to load/use other classes in other 
loaders/modules.

> There are at least two possibilities if someone wants to get a
> System.Logger from a JNI attached thread:
>
>   1. Use an auxilliary class to indicate the implicit caller (that
>      will be the auxilliary class module)
>
>   2. Call LoggerFinder.getLoggerFinder().getLogger and explicitly supply
>      a module
>
> IMO calling directly System.getLogger is not appropriate in this case.

If you are going to introduce an API with such a constraint then the 
constraint needs to be clearly documented and the alternatives also 
documented IMHO. System.getLogger relies on a notion of "current module".

Cheers,
David

> best,
>
> -- daniel
>
>>
>> Mandy
>>
>>> On Mar 20, 2017, at 5:08 AM, Daniel Fuchs <daniel.fuchs at oracle.com>
>>> wrote:
>>>
>>> Hi,
>>>
>>> Please find below a patch for:
>>>
>>> 8177136: Caller sensitive methods Logger.getLogger,
>>> Logger.getAnonymousLogger, and System.getLogger should throw
>>> IllegalCallerException if there is no caller on the stack.
>>> https://bugs.openjdk.java.net/browse/JDK-8177136
>>>
>>> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
>>> and System.getLogger currently throw an undocumented
>>> NullPointerException if they are called from JNI and there is no
>>> Java frame on the stack.
>>>
>>> Throwing NullPointerException is confusing and makes it look like
>>> there is a bug in the implementation.
>>> In truth, these method are @CallerSensitive, and therefore must not
>>> be called in a context where the caller cannot be determined.
>>> Therefore, the right thing to do is to throw IllegalCallerException
>>> and document this.
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>>>
>>> As per Rampdown Phase 2 Process [1] I'd also like to get
>>> confirmation that this is a reasonable proposal to fix in 9.
>>> This fix just transmutes a NullPointerException (which should
>>> never happen at this point in regular usage of the API) into an
>>> IllegalCallerException which will help diagnosing the fact
>>> that the API is called from a context where it's not supposed
>>> to be used. The risk of fixing should therefore be very limited.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> [1] Rampdown Phase 2 Process
>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html
>>
>