Code review: 8010464: Evolve java networking same origin policy

[Michael McMahon]

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/2b8f0a8f/attachment.html