Code review: 8010464: Evolve java networking same origin policy
Michael McMahon
michael.x.mcmahon at oracle.com
Mon May 13 08:28:33 PDT 2013
No, the class is used for both request and response headers.
Also, Arrays.asList() doesn't work in this case because we have to
account for the number
of elements used. So, something like what you are suggesting below
should work.
Michael
On 13/05/13 16:10, Vitaly Davidovich wrote:
>
> If the headers are supposed to be parsed and then readonly, then the
> class can be made immutable? Don't know how much work that would entail.
>
> To minimize synch overhead, I think you can assign the array and nkeys
> to locals inside a synch region and then do the copying outside of it.
>
> Sent from my phone
>
> On May 13, 2013 10:43 AM, "Michael McMahon"
> <michael.x.mcmahon at oracle.com <mailto:michael.x.mcmahon at oracle.com>>
> wrote:
>
> Okay, adding or removing elements would do that (though it won't
> happen in practice)
> as opposed to reset(). So, I guess it's better practice to make it
> synchronized.
>
> Thanks
> Michael.
>
> On 13/05/13 15:19, Vitaly Davidovich wrote:
>>
>> If the array or nkeys is modified while getHeaders is running,
>> you can get NPE or ArrayIndexOutOfBoundsException. If you
>> synchronize, the method returns some list of headers, and it's
>> true that as soon as it returns some other thread can mutate the
>> headers and thus the returned list isn't "in-sync" with current
>> headers, but there's no exception.
>>
>> Sent from my phone
>>
>> On May 13, 2013 9:45 AM, "Michael McMahon"
>> <michael.x.mcmahon at oracle.com
>> <mailto:michael.x.mcmahon at oracle.com>> wrote:
>>
>> On 13/05/13 14:12, Vitaly Davidovich wrote:
>>>
>>> I get what you're saying about before/after, but the
>>> difference would be that if it's called during then you get
>>> an exception purely due to missing synchronization; in the
>>> before/after case, caller may observe "stale" entries but
>>> that's fine, as you say.
>>>
>>
>> How would that be? The only effect of synchronization is to
>> ensure that the other call
>> occurs before or after (so to speak).
>>
>>> Maybe headers aren't reset in practice, but this code looks
>>> suspect to someone reading it. :)
>>>
>>
>> Right, we shouldn't be depending on caller behavior, but I
>> still don't see a problem to be fixed.
>>
>> Michael
>>
>>> Sent from my phone
>>>
>>> On May 13, 2013 9:05 AM, "Michael McMahon"
>>> <michael.x.mcmahon at oracle.com
>>> <mailto:michael.x.mcmahon at oracle.com>> wrote:
>>>
>>> Hi Vitali,
>>>
>>> I was going to switch to use Arrays.asList() as you and
>>> Alan suggested. So getHeaderNames() would look like:
>>>
>>> public List<String> getHeaderNames() {
>>> return Arrays.asList(keys);
>>> }
>>>
>>> So, it turns out synchronizing nkeys and keys is no
>>> longer necessary.
>>> It's true that reset() could be called during the call.
>>> But, it could (in theory) be called
>>> before or after either. In practice that won't happen,
>>> since the request headers
>>> aren't ever reset.
>>>
>>> Michael
>>>
>>>
>>>
>>> On 13/05/13 13:36, Vitaly Davidovich wrote:
>>>>
>>>> Actually, local may not work since getHeaders uses
>>>> nkeys as well - can run into AIOBE. Probably best to
>>>> just synchronize given current implementation.
>>>>
>>>> Sent from my phone
>>>>
>>>> On May 13, 2013 8:30 AM, "Vitaly Davidovich"
>>>> <vitalyd at gmail.com <mailto:vitalyd at gmail.com>> wrote:
>>>>
>>>> Hi Michael,
>>>>
>>>> On the synchronized issue, I think you do need it;
>>>> if someone, e.g., calls reset() while this method
>>>> is running, you'll get NPE. Maybe pull the keys
>>>> array into a local then and iterate over the local
>>>> instead?
>>>>
>>>> Also, why LinkedList instead of ArrayList(or
>>>> Arrays.asList, as Alan mentioned, although maybe
>>>> caller is expected to modify the returned list)?
>>>>
>>>> Thanks
>>>>
>>>> Sent from my phone
>>>>
>>>> On May 13, 2013 6:42 AM, "Michael McMahon"
>>>> <michael.x.mcmahon at oracle.com
>>>> <mailto:michael.x.mcmahon at oracle.com>> wrote:
>>>>
>>>> Thanks for the review. On the javadoc comments,
>>>> there are a couple
>>>> of small spec changes that will probably happen
>>>> after feature freeze anyway.
>>>> So, that might be the best time to address the
>>>> other javadoc issues.
>>>>
>>>> I agree with your other comments. On the
>>>> synchronized method in MessageHeader,
>>>> I don't believe it needs to be synchronized
>>>> since the method is not relying on
>>>> consistency between object fields, and the
>>>> returned object can be
>>>> modified before, during or after the method is
>>>> called anyway.
>>>>
>>>> Michael
>>>>
>>>> On 12/05/13 08:13, Alan Bateman wrote:
>>>>
>>>> On 10/05/2013 12:34, Michael McMahon wrote:
>>>>
>>>> Hi,
>>>>
>>>> This is the webrev for the
>>>> HttpURLPermission addition.
>>>> As well as the new permission class,
>>>> the change
>>>> includes the use of the permission in
>>>> java.net.HttpURLConnection.
>>>>
>>>> The code basically checks for a
>>>> HttpURLPermission in plainConnect(),
>>>> getInputStream() and getOutputStream()
>>>> for the request and if
>>>> the caller has permission the request
>>>> is executed in a doPrivileged()
>>>> block. When the limited doPrivileged
>>>> feature is integrated, I will
>>>> change the doPrivileged() call to limit
>>>> the privilege elevation to a single
>>>> SocketPermission (as shown in the code
>>>> comments).
>>>>
>>>> The webrev is at
>>>> http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
>>>> <http://cr.openjdk.java.net/%7Emichaelm/8010464/webrev.1/>
>>>>
>>>> A partial review, focusing mostly on the
>>>> spec as we've been through a few rounds on
>>>> that part already. Overall I think the
>>>> javadoc looks quite good. I realize someone
>>>> suggested using lowercase "url" in the
>>>> javadoc but as the usage is as an acronym
>>>> then it might be clearer if it were in
>>>> uppercase, maybe "URL string" to avoid any
>>>> confusion with java.net.URL.
>>>>
>>>> I assume you'll add a copyright header to
>>>> HttpURLPermission before pushing this.
>>>>
>>>> A minor comment on the javadoc tags is that
>>>> you probably should use @throws instead of
>>>> @exception.
>>>>
>>>> At a high-level it would be nice if the
>>>> fields were final but I guess the parsing
>>>> of actions and being serialized complicates
>>>> this.
>>>>
>>>> setURI - this parses the URI rather than
>>>> "sets" it so maybe it should be renamed. If
>>>> you use URI.create then it would avoid
>>>> needing to catch the URISyntaxException.
>>>>
>>>> normalizeMethods/normalizeHeaders- I assume
>>>> these could use an ArrayList.
>>>>
>>>> HttpURLConnection - "if a security manager
>>>> is installed", should this be "set"?
>>>>
>>>> MessageHeader - some of the methods are
>>>> synchronized, some are not. I can't quite
>>>> tell if getHeaderNames needs to be
>>>> synchronized. Also is there any reason why
>>>> this can't use Arrays.asList?
>>>>
>>>> HttpURLConnection.setRequestMethod -
>>>> "connection being open" -> "connect in
>>>> progress"?
>>>>
>>>> That's all I have for now but I think there
>>>> is further review work required on
>>>> HttpURLConnection as some of that is tricky.
>>>>
>>>> -Alan.
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/net-dev/attachments/20130513/f2d59628/attachment.html
More information about the net-dev
mailing list