RFR: JDK-6515161 If remote removeNotificationListener gets SecurityException, client no longer gets notifications

[Daniel Fuchs]

+1

Thanks Ujwal!

-- daniel

On 09/05/17 06:38, Ujwal Vangapally wrote:
> Hi Harsha, Daniel,
> 
> updated method names as getListenerIds/getListenerId
> 
> webrev : 
> http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.02/
> 
> 
> On 5/8/2017 9:57 PM, Daniel Fuchs wrote:
>> Hi Harsha,
>>
>> Well, I think both methods should have the same name
>> and I don't see any strong reason for changing the old
>> name - since there is no such thing as a 'ClientListener'
>> (there are only NotificationListeners).
>>
>> best regards,
>>
>> -- daniel
>>
>> On 08/05/2017 17:13, Harsha Wardhana B wrote:
>>> Hi Daniel,
>>>
>>> The term 'listenerid' is used in conjunction with method name/object
>>> field which adds context about the term 'listenerid'. Having a
>>> standalone method name as getClientListenerId is less ambiguous compared
>>> to method name : getListenerId.
>>>
>>> I don't really have a strong opinion on this and am okay with either 
>>> names.
>>>
>>> -Harsha
>>>
>>>
>>> On Monday 08 May 2017 03:16 PM, Daniel Fuchs wrote:
>>>> On 08/05/2017 09:45, Ujwal Vangapally wrote:
>>>>> Thanks for the Review Daniel, Harsha
>>>>>
>>>>> Please find the new webrev incorporating the review comments
>>>>>
>>>>> webrev :
>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.01/
>>>>
>>>> Looks good. In retrospect I wonder if renaming
>>>> getListenerIds/getListenerId into
>>>> getClientListenerIds/getClientListenerId
>>>> was such a good idea given that 'listenerId' is an established
>>>> terminology in the specification [1] [2] (and 'listenerId' is used all
>>>> over the place in ClientNotifForwarder anyway).
>>>>
>>>> Harsha, what do you think?
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> [1] See @return in
>>>> http://download.java.net/java/jdk9/docs/api/javax/management/remote/rmi/RMIConnection.html#addNotificationListeners-javax.management.ObjectName:A-java.rmi.MarshalledObject:A-javax.security.auth.Subject:A- 
>>>>
>>>>
>>>> [2] see @param listenerIds in
>>>> http://download.java.net/java/jdk9/docs/api/javax/management/remote/rmi/RMIConnection.html#removeNotificationListeners-javax.management.ObjectName-java.lang.Integer:A-javax.security.auth.Subject- 
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Ujwal
>>>>>
>>>>>
>>>>> On 5/8/2017 12:03 PM, Daniel Fuchs wrote:
>>>>>> Hi Ujwal,
>>>>>>
>>>>>> For consistency I think the new getListenerIds method should:
>>>>>>
>>>>>> a) either return an array of Integer, even if it contains only 1
>>>>>> Integer:
>>>>>>
>>>>>>   1. The name of the method implies that an array is returned
>>>>>>   2. You will need the array when you call
>>>>>>      connection.removeNotificationListeners anyway.
>>>>>>
>>>>>> or b) the other possibility is to remove the 's' at the end of the
>>>>>> method.
>>>>>>
>>>>>> Both would be acceptable.
>>>>>>
>>>>>> best regards,
>>>>>>
>>>>>> -- daniel
>>>>>>
>>>>>> On 04/05/17 07:59, Ujwal Vangapally wrote:
>>>>>>> corrected webrev link :
>>>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 5/4/2017 12:14 PM, Ujwal Vangapally wrote:
>>>>>>>>
>>>>>>>> Kindly review the changes made for below bug
>>>>>>>>
>>>>>>>> Problem description and solution are explained in comments section
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6515161
>>>>>>>>
>>>>>>>> diff for*ClientNotifForwarder.java *might be a bit confusing as it
>>>>>>>> shows the method name
>>>>>>>>
>>>>>>>> removeNotificationListener is modified to getListenerIds and new
>>>>>>>> method removeNotificationListener is introduced.
>>>>>>>>
>>>>>>>> Actual change is new method getListenerIds is introduced and it is
>>>>>>>> called in removeNotificationListener method without
>>>>>>>>
>>>>>>>> affecting the functionality of removeNotificationListener.
>>>>>>>>
>>>>>>>> webrev :
>>>>>>>> cr.openjdk.java.net/~uvangapally/webrev/2017/6515161/webrev.00/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Ujwal,
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>