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.
