Immutable HttpFields and MetaData

[gregw]

This is an experimental branch to see the impact of making MetaData immutable. This is currently against 10, but may be for a 10.1 or later version.

[gregw]

First few commits are about creating an immutable HttpFields. The HttpField class itself was already immutable.

[gregw]

@joakime @sbordet @janbartel @lorban @lachlan-roberts I've made HttpFields, HttpURI and MetaData immutable and fixed things up to the http module. It's going to take a while to get everything else fixed, but would be good to get feedback before I commit too much more effort if the tree I'm barking up is not going to work. So if you wanted to review those three classes whilst this is a broken WIP, I'd appreciate any comments etc. (ok not any comments... )

[lorban]

There's a lack of coherency to quickly figure out if a class is immutable or not or can be made immutable. Maybe we should introduce some form of common pattern for classes that can sometimes be mutable or immutable (like the collections' Collection.unmodifiable() for instance) and/or a naming convention to distinguish them?

[gregw]

I've pushed changes that makes tests pass up to and including jetty-server. So I think it is worthwhile tacking step back and reviewing the approaches, which as @lorban says, have a "lack of coherency"

Currently I've taken 3 different approaches to immutability:

1. HttpURI is now immutable itself and has an explicit Builder for mutable operations like:

   HttpURI newUri = new HttpURI.Builder(oldUri).host(serverName).port(serverPort).normalize().build();

2. HttpFields has taken the opposite approach, keeping that class as a mutable implementing the current API. It has a method asImmutable() that returns a HttpFieldList, which is a new interface that has all the read only methods of HttpFields. This interface is implemented by both HttpFields and it's private HttpFields.Immutable implementation. Because their is no builder pattern, the code to modify is a bit more verbose because there is no chaining:

   HttpFields newFields = new HttpFields(oldFieldList);
   newFields.remove(someHeader);
   newFields.put("Header", "replacement value");
   HttpFieldList newFieldList = newFields.asImmutable();

3. MetaData, MetaData.Request and MetaData.Response are purely immutable with no builder and only constructors. This is somewhat of a pain in the server as http/1 parser delivers all the components in separate callbacks, so they need to be remembered before making the metadata instance when the headers are complete. This could be a bit more elegant and readable with either a builder pattern or a mutable version of itself.

One immutable variation I have not used is having an API that is read/write, but the immutable version returns a UnsupportedOperationException for write methods. This could perhaps tidy up the HttpFields approach but only if we were able to make HttpFields an interface which would break backwards compatibility.

Also I don't particularly like the Builder pattern being so explicit for HttpURI, so perhaps that could be made a bit cleaning with some utility methods so you could write:

   HttpURI newUri = HttpURI.copy(oldUri).host(serverName).port(serverPort).normalize().toHttpURI();

Perhaps this approach could also be used to add a builder to HttpFields:

   HttpFields.copy(oldFields).remove(someHeader).put("Header","replace value").toHttpFields();

This would make HttpFields immutable, so it would not be possible to have code like:

   HttpFields fields = new HttpFields();
   fields.add("Header", "value");
   fields.add("Host", "localhost");

This would instead need to be written as

   HttpFields fields = HttpFields.empty()
       .add("Header", "value")
       .add("Host", "localhost")
       .toHttpFields();

If we decide to go that way, then I think the same approach would work for MetaData.

I may have a fiddle with this.... but I'd really like a bit more feedback before I commit too much more effort. @sbordet specially for h2 and the client.

[lorban]

For your inspiration, I was thinking about using a pair of marker interfaces that would explicitly state what classes are builders and how to derive a new object from a builder-generated one.

Something like:

public interface Builder<T>
{
  T build();
}

public interface Derivable<B extends Builder<? extends Derivable<?>>>
{
  B derive();
}

which would give us:

public class HttpFieldsBuilder implements Builder<HttpFields>
{
  public HttpFields build()
  {
  ...
  }
}

public class HttpFields implements Derivable<HttpFieldsBuilder>
{
  public HttpFieldsBuilder derive()
  {
  ...
  }
}

public class Sample
{
  public static void main()
  {
    HttpFields original = new HttpFieldsBuilder()
      .add("Header", "value")
      .add("Host", "localhost")
      .build();

    HttpFields copy = original.derive()
      .remove("Header")
      .put("Port", "8080")
      .build();
  }
}

These interfaces would have minimal added value, but would mostly serve as "marker" interfaces, explicitly letting the programmer know that using some class requires following the common builder / derive pattern.

[gregw]

@sbordet can I get a few hours of your time early next week to consider the impact of this on the client and h2.

[sbordet]

Almost there.

[gregw]

@sbordet are we there yet?

[joakime]

I'm happy with this too.

[joakime]

Last build failed.

[joakime]

Can you merge in the current jetty-10.0.x and let CI build cleanly once?