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.mcma...@oracle.com <mailto:michael.x.mcma...@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.mcma...@oracle.com
<mailto:michael.x.mcma...@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"
<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.
