[9] RFR: 8151182: HttpHeaders.allValues should return unmodifiable List as per JavaDoc

[Michael McMahon]

Yes, there is a mutability test already. We will have to fix the thread 
safety problem
(and also the fact HttpHeaders1 was left public by mistake). Probably will
separate the mutable and immutable types completely. Vaibhav, if you'd 
like to do it,
you can define a package private implementation of HttpHeaders whose 
constructor
takes a HttpHeadersImpl and uses final fields to ensure thread safety, 
rather than
using makeUnmodifiable(). I'll contact you with some other ideas.

I'll open a new report on this point and we probably should review on 
net-dev

- Michael.


On 04/03/16 14:05, Paul Sandoz wrote:
> Hi Vaibhav,
>
> This will not work, as Claes points out. You also might wanna check if there are tests in place asserting unmodfiablity, if there are none you could add some. Michael can point you in the right direction.
>
> Follow the trail of HttpHeaders1.makeUnmodifiable(), which is used to transition map values from mutable to unmodifiable:
>
>      @Override
>      public void makeUnmodifiable() {
>          if (isUnmodifiable)
>              return;
>
>          Set<String> keys = new HashSet<>(headers.keySet());
>          for (String key : keys) {
>              List<String> values = headers.remove(key);
>              if (values != null) {
>                  headers.put(key, Collections.unmodifiableList(values));
>              }
>          }
>          isUnmodifiable = true;
>      }
>
> In fact duplication of the key set can be avoided if one does this:
>
>    Iterator<Map.Entry<…>> ie = headers.entrySet().iterator();
>    while (ie.hashNext()) {
>      Map.Entry<…> e = ie.next();
>      if (e.getValue() != null) {
>        e.setValue(Collections.unmodifiableList(e.getValue()));
>      }
>      else {
>        ie.remove();
>      }
>    }
>
> However, i suspect this could be simplified to:
>
>    headers.replaceAll((k, v) -> Collections.unmodifiableList(v)))
>
> If there are never any explicit null values placed in the map, which should be the case as that is really an anti-pattern.
>
> Also this:
>
>      private List<String> getOrCreate(String name) {
>          List<String> l = headers.get(name);
>          if (l == null) {
>              l = new LinkedList<>();
>              headers.put(name, l);
>          }
>          return l;
>      }
>
> can be replaced with this:
>
>    return headers.computeIfAbsent(name, k -> new LinkedList<>());
>
> Paul.
>
>> On 4 Mar 2016, at 14:19, vaibhav x.choudhary <vaibhav.x.choudhary at oracle.com> wrote:
>>
>> Hi,
>>
>> Please review :-
>>
>> Review Link :- http://cr.openjdk.java.net/~ntv/vaibhav/JDK8151182/webrev.00/
>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8151182
>>
>> --
>> Thank You,
>> Vaibhav Choudhary
>> http://blogs.oracle.com/vaibhav
>>