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" <vita...@gmail.com
<mailto: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
<mailto: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/%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.
