Chris, On HttpRequest.
While everything is functioning as expected, it may be possible to clean up the HttpRequest instances built through the builder, making it easier to reason about the code. It seems like a straight forward change to have WebSocket use an internal API to create its requests. That makes it possible to remove all but the minimal amount of state from requests built through the builder. This is all possible, with minimal changes, since sending a request needs to perform a defensive copy. Much cleaner. http://cr.openjdk.java.net/~chegar/http_immutableHttpRequest/ -Chris. > On 22 May 2018, at 20:05, Chris Povirk <cpov...@google.com> wrote: > > ... > > > * I'm a little confused by HttpRequest. At first, I thought it was a > value type like HttpHeaders, so I was going to have the same > suggestion of making it a final type (and also its builder). But the > implementation appears to be mutable and to contain additional > fields. It feels like the type is pulling double duty as both a > value type in the public API (which could be nice to make final) and > as a stateful internal type (which might contain an instance of the > public type), and it's not clear if different implementations could > successfully interchange HttpRequest instances. (Tobias said that > he'd raised this point earlier, and it sounds like it may > realistically have to stay how it is, but since this was my reaction > and it turned out to match what he'd said, I wanted to at least > mention it again.) > > > From an API perspective HttpRequest is immutable, and all > implementations returned from the client exhibit this behaviour. If not, > then it's a bug. I think, since you clearly looked at the > implementation, that you noticed that the HTTP request implementation > has some mutable state that it carries, that is required for certain > parts of the default implementation. That is not unusual or any cause > for alarm, and does not have any impact on the exposed state. > > I was wondering if our differences here might hinge on the definition of > state, which is interesting > <http://james-iry.blogspot.com/2009/04/state-of-sock-tubes.html> but might > lead too far afield. > > But it might be simpler than that. More concretely, what I was looking at is > that isWebSocket is mutated after construction. (I also saw some other > non-final fields, but those appear to never be modified after construction.) > This suggested to me that I couldn't create an HttpRequest and then send it > multiple times (possibly concurrently), since one attempt's update to > isWebSocket might interfere with the other. (It at least appears that > isWebSocket is only ever changed from false to true, never the reverse.) But > I haven't traced through the WebSocket APIs to see if it's actually possible > for a given HttpRequest instance to be used both with and without web > sockets. If it's not possible, then I don't see any way to trigger a problem > based on the state -- which might be what you were getting at. And I admit > that it's unlikely that anyone would use a single instance both with an > without web sockets, anyway. I certainly was reacting more to the mere idea > of a non-final field than to any concrete problem I can produce. (And I may > have assumed that Tobias's earlier comments referred to the same field, not > to some earlier bits of state that you've since corrected. Sorry about that.) > > There can be different HttpRequest implementations. For example, the > default implementation disallows requests with the CONNECT method - > which is used for tunneling - and there are restrictions around its use > when performing security manager permission checks. Another > implementation may not have such a restriction. There are a few other > small constraints and restrictions specific to the default client that > another implementation may choose to do differently. > > It is for these reasons that neither HttpRequest or its Builder are > final. More generally, there is a clear tension between extensibility > and finality. We favour the former to be more friendly to other > implementations, and to support scenarios like offline testing ( and to > a lesser extent mocking ). >
