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" <vita...@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.mcma...@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/~michaelm/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. >>> >> >>
