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

[Daniel Fuchs]

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,
>>>>>>
>>>>>
>>>>
>>>
>>
>