Code review: 8010464: Evolve java networking same origin policy
Michael McMahon
michael.x.mcmahon at oracle.com
Mon May 13 07:43:40 PDT 2013
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/26d87ca3/attachment.html
More information about the net-dev
mailing list