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>
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>
> 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>
>> 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.
>>>>>>
>>>>>
>>>>>
>>>
>>
>
