Code review request, JDK-8010815, some constructors issues in com.sun.jndi.toolkit
David Holmes
david.holmes at oracle.com
Tue May 28 02:17:32 UTC 2013
I'm not familiar with these particular APIs but in general making
defensive copies of mutable objects is good practice. In this case
however it is unclear what the impact may be on existing users of this
code. Eg:
com/sun/jndi/toolkit/ctx/Continuation.java
The "environment" is only stored for later use with the
CannotProceedException, at which time the environment is cloned anyway
(and cloned again by at least some consumers of that object!). So does
it matter that we store a reference to the external object? To me that
is more of a concern for the caller "will somebody mess with this if I
pass it in?" - they might even be passing you a clone in the first
place! If someone is passing in an environment that is subsequently
modified, this change will stop that modification from being seen -
which might be a good thing and might be a bad thing. There is also the
memory usage consideration (how big are these environment tables?).
So in terms of the mechanics of making defensive copies this code seems
fine. But in terms of whether you _should_ be making defensive copies,
that is a much harder question that I can't answer.
David
-----
On 28/05/2013 11:31 AM, Xuelei Fan wrote:
> Ping again, any one available to review this changeset?
>
> Thanks,
> Xuelei
>
> On 5/13/2013 11:06 PM, Xuelei Fan wrote:
>> On 5/13/2013 10:52 PM, Wang Weijun wrote:
>>> I'll read them tomorrow.
>> OK. It's not a urgent fix.
>>
>>> One question: are all of them designed to be call-by-value instead of shareable?
>>>
>> I'm not sure I understand the term "call-by-value" or "shareable". From
>> my understand, they are public classes, and copy-on-write or share-value
>> is the not right solution. Cloning mutable objects is more robust.
>> Otherwise, we need to check every caller to the method and the thread
>> stacks to make sure it is not abused. It's nightmare to maintain such code.
>>
>> In the JNDI specification, we normally declare like "This constructor
>> will not modify environment or save a reference to it, but may save a
>> clone. Caller should not modify mutable keys and values in environment
>> after it has been passed to the constructor."
>>
>> Thanks,
>> Xuelei
>>
>>> Thanks
>>> Max
>>>
>>> On May 13, 2013, at 10:05 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> There is some constructors issues about mutable objects in
>>>> com.sun.jndi.toolkit.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~xuelei/8010815/webrev.00/
>>>>
>>>> Thanks,
>>>> Xuelei
>>
>
More information about the core-libs-dev
mailing list