No, the class is used for both request and response headers.
Also, Arrays.asList() doesn't work in this case because we have to
account for the number
of elements used. So, something like what you are suggesting below
should work.
Michael
On 13/05/13 16:10, Vitaly Davidovich wrote:
If the headers are supposed to be parsed and then readonly, then the
class can be made immutable? Don't know how much work that would entail.
To minimize synch overhead, I think you can assign the array and nkeys
to locals inside a synch region and then do the copying outside of it.
Sent from my phone
On May 13, 2013 10:43 AM, "Michael McMahon"
<michael.x.mcma...@oracle.com <mailto:michael.x.mcma...@oracle.com>>
wrote:
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.
