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.
Maybe headers aren't reset in practice, but this code looks suspect to
someone reading it. :)
Sent from my phone
On May 13, 2013 9:05 AM, "Michael McMahon" <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> 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/
>>>>>
>>>> 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.
>>>>
>>>
>>>
>
