ring icon indicating copy to clipboard operation
ring copied to clipboard

Ring 2.0 feedback

Open weavejester opened this issue 4 years ago • 143 comments

This is an issue for gathering feedback on the Ring 2.0 design.

The code and documentation will be held in the 2.0 branch of the repository.

The first draft specification for Ring 2.0 is now available to read, and accompanying it is an architecture design record that attempts to explain the decision-making behind the design.

New features for Ring 2.0 include:

  • Namespaced request and response maps
  • Request body changed from Java InputStream to protocol
  • Websockets
  • HTTP/2 push promises

Note that Ring does not use semantic versioning. Ring 2.0 will be backward compatible with Ring 1.0, as described in the ADR.

As of writing this design has not yet been implemented in code. The next step is to create an alpha release that implements the draft spec, allowing the community to try the design out. I anticipate Ring 2.0 will remain in alpha for a while, in a similar fashion to Clojure spec, to allow plenty of time for community feedback and testing.

weavejester avatar Feb 28 '20 15:02 weavejester

:tada:

Did you mean :ring.request/body -> :ring.response/body etc in the table? image

iku000888 avatar Feb 28 '20 15:02 iku000888

  1. Why not use core.async protocols to async responses?
  2. ring.request keys and ring.push keys has identical specs. Why not reuse ring.request in push maps?
  3. Let's use clojure.core predicates in "types"? I mean, use string? where was String

souenzzo avatar Feb 28 '20 15:02 souenzzo

Why not use core.async protocols to async responses?

It adds a dependency on core.async.

ring.request keys and ring.push keys has identical specs. Why not reuse ring.request in push maps?

So that we can distinguish between a push map and a request map. A push map also has different mandatory keys to a request map.

Let's use clojure.core predicates in "types"? I mean, use string? where was String

That doesn't work for protocols, and I'd rather be precise and use a type over a predicate. There are some predicates that don't map 1-to-1 with types.

weavejester avatar Feb 28 '20 15:02 weavejester

Did you mean :ring.request/body -> :ring.response/body etc in the table?

Yes, good catch. Let me fix that.

weavejester avatar Feb 28 '20 15:02 weavejester

Why not use core.async protocols to async responses?

It adds a dependency on core.async.

So can we consider a new protocol for async API? Then we can easily bridge any async lib into ring protocol.

ring.request keys and ring.push keys has identical specs. Why not reuse ring.request in push maps?

So that we can distinguish between a push map and a request map. A push map also has different mandatory keys to a request map.

Both clojure.spec and plumatic/schema has capabilities to say with keys are required in different contexts. In clojure.spec example:

(s/def ::request (s/keys :req [:ring.request/method]
                         :opt [:ring.request/path]))
(s/def ::push (s/keys :req [:ring.request/path]
                      :opt [:ring.request/method]))

Let's use clojure.core predicates in "types"? I mean, use string? where was String

That doesn't work for protocols, and I'd rather be precise and use a type over a predicate. There are some predicates that don't map 1-to-1 with types.

ring can define ring.response/streamable-response-body? next to the protocol. Things like Integer don't make sense in some targets like clojurescript/JS, I think that :ring.response/status for example, can be a number?

souenzzo avatar Feb 28 '20 16:02 souenzzo

So can we consider a new protocol for async API? Then we can easily bridge any async lib into ring protocol.

Can you explain what you mean by "for async API"? Do you mean adding a protocol for non-blocking I/O on the request and response bodies?

Both clojure.spec and plumatic/schema has capabilities to say with keys are required in different contexts.

Sure, but that doesn't help if we want to differentiate between a push promise and a HTTP request dynamically. A push promise also has different semantic meaning to a HTTP request, even though they share fields of the same type.

Things like Integer don't make sense in some targets like clojurescript/JS, I think that :ring.response/status for example, can be a number?

The StreamableResponseBody protocol doesn't make sense for ClojureScript either. The specification is designed to target Clojure, rather than ClojureCLR or ClojureScript.

I have thought about what we'd do to adapt the specification to ClojureScript, and it goes a little deeper than just using predicates rather than type names.

weavejester avatar Feb 28 '20 16:02 weavejester

How about async streaming API - if my handler wants to send chunks?

niquola avatar Feb 28 '20 17:02 niquola

We use a lot a context map (with bunch of deps like db connection etc) as a first parameter to handlers - may it be useful for ring?

niquola avatar Feb 28 '20 18:02 niquola

The spec obviously spends time describing the differences between the synchronous and asynchronous protocols. One thing it doesn’t seem to mention is the serialization of requests. For instance, can a server have multiple synchronous requests in flight at the same time, or does synchronous imply that requests are serialized, one after another? In particular, if a synchronous handler blocks, is it blocking all requests, or just the request that it’s servicing? Perhaps the omission is on purpose to allow for different server adapter implementations, but the silence is noticeable. If it’s purposeful, perhaps a statement to that effect would be good to include.

dgr avatar Feb 28 '20 18:02 dgr

How about async streaming API - if my handler wants to send chunks?

That was added back in Ring 1.6, with the caveat that I/O is blocking. In other words, while your handler is actively writing data to the client it's blocking the thread.

The problem with fully supporting non-blocking I/O in Ring 1 was that the request body was an InputStream, which is a blocking stream. In Ring 2, that's been changed, and in Ring 2.1 the plan is to have NIO protocols that the request body and response body can optionally satisfy.

For Ring 2.0 I thought that sort of performance optimisation was a little out of scope, as it's a fairly niche requirement.

We use a lot a context map (with bunch of deps like db connection etc) as a first parameter to handlers - may it be useful for ring?

In general I think it's generally better to use closures for that:

(fn [context]
  (fn [request]
    ...))

But I'd be interested in hearing about use-cases where closures are not as elegant. That said, I don't anticipate changing the request/response maps into context maps instead.

weavejester avatar Feb 28 '20 18:02 weavejester

For instance, can a server have multiple synchronous requests in flight at the same time, or does synchronous imply that requests are serialized, one after another? In particular, if a synchronous handler blocks, is it blocking all requests, or just the request that it’s servicing?

No, it's just blocking the thread for the current request. It's expected that an adapter that is running a synchronous handler will have a threadpool for handling multiple requests concurrently.

weavejester avatar Feb 28 '20 18:02 weavejester

For instance, can a server have multiple synchronous requests in flight at the same time, or does synchronous imply that requests are serialized, one after another? In particular, if a synchronous handler blocks, is it blocking all requests, or just the request that it’s servicing?

No, it's just blocking the thread for the current request. It's expected that an adapter that is running a synchronous handler will have a threadpool for handling multiple requests concurrently.

OK. My suggestion would be to make that explicit so that nobody has to guess. I assume that the size of the thread pool is really in the domain of the server adapter and shouldn't be part of the Ring spec, but there's a big difference between one (serial) and more than one (non-serial). I'm sure that Ring 1.x behavior already works this way, but given the chance to document it explicitly, I would take the opportunity.

dgr avatar Feb 28 '20 18:02 dgr

This does make accessing request headers with single values slightly more laborious: (first (get-in request [:headers "content-type"]))

If you are already using get-in, it’s still simple:

(get-in request [:headers "content-type" 0])

tonsky avatar Feb 28 '20 18:02 tonsky

The spec does a great job of considering HTTP/2 issues. Has it been checked against HTTP/3 proposals? I realize that's a moving target and still in flux, but it seems like 2020 is the year that HTTP/3 will start to happen. It's already included in stable versions of Chrome and Firefox, though not the default, per https://en.wikipedia.org/wiki/HTTP/3.

dgr avatar Feb 28 '20 19:02 dgr

Great spec, all changes very solid and timely!

tonsky avatar Feb 28 '20 19:02 tonsky

BTW, I should also add, great job on Ring 2 and in documenting the rationale behind the decisions. It was a pleasure to read through these documents and I can't wait to use it in anger. Ring is one of the most important APIs in the Clojure ecosystem and you've done an amazing job both with Ring 1 and the evolution to Ring 2, James. Thanks for all your effort over the years.

dgr avatar Feb 28 '20 19:02 dgr

By contrast, Ring 2 mandates that response header names be lowercase, and the values be vectors:

Is it required for the spec implementor (a web server) to validate this?

An key benefit to being able to identify requests and responses is that we can retain backward compatibility across middleware and utility functions. We can efficiently distinguish between requests that adhere to the 1.x specification, and those that adhere to the 2.x specification, and invoke different behavior depending on the type of input.

I can't help looking at such backward compatibility from the performance perspective. Would this mean that a spec implementation must expose both sets of keys to the middleware/handlers? From a brief glimpse, it feels like this can bring a noticeable overhead from growing the request map, allocating extra objects for the header values (the smallest vector is 240 bytes, btw), and so on.

alexander-yakushev avatar Feb 28 '20 21:02 alexander-yakushev

I would argue that websockets should not be supported until NIO has landed. WS is one of the strongest use cases for NIO, as a lot of applications have lots of mostly idle connections. Without NIO this is pretty inefficient due to the overheads in managing those connections.

SevereOverfl0w avatar Feb 28 '20 23:02 SevereOverfl0w

My suggestion would be to make that explicit so that nobody has to guess. I assume that the size of the thread pool is really in the domain of the server adapter and shouldn't be part of the Ring spec, but there's a big difference between one (serial) and more than one (non-serial).

I'll consider adding a note explaining the terminology.

weavejester avatar Feb 29 '20 00:02 weavejester

Has it been checked against HTTP/3 proposals?

My understanding is that the main difference between HTTP/2 and HTTP/3 is the introduction of QUIC as a transport protocol, which doesn't affect Ring.

weavejester avatar Feb 29 '20 00:02 weavejester

By contrast, Ring 2 mandates that response header names be lowercase, and the values be vectors:

Is it required for the spec implementor (a web server) to validate this?

No, because by the time it gets to the adapter it's essentially too late. The idea is to make it simpler to write middleware that reads response headers.

The adapter will probably throw an exception if it sees a string where it expects a vector, but it's not the adapter's responsibility to check the header strings are lowercase. Unless it really wants to.

I can't help looking at such backward compatibility from the performance perspective. Would this mean that a spec implementation must expose both sets of keys to the middleware/handlers?

No; an adapter would be started in Ring 1 or Ring 2 mode. Middleware would be able to handle both types, in the same way that middleware can currently handle asynchronous or synchronous handlers.

CPU performance impact is likely to be small; around 6ns added to each request per middleware, so we're talking under 100ns. This may be further reduced by CPU branch prediction, as the same condition branch will be hit each time.

From a brief glimpse, it feels like this can bring a noticeable overhead from growing the request map, allocating extra objects for the header values (the smallest vector is 240 bytes, btw), and so on.

In terms of larger request maps, yes this is true. However, it's far quicker to perform a lookup on a vector than to parse a string, so we'll save some CPU cycles in exchange.

weavejester avatar Feb 29 '20 00:02 weavejester

I would argue that websockets should not be supported until NIO has landed. WS is one of the strongest use cases for NIO, as a lot of applications have lots of mostly idle connections. Without NIO this is pretty inefficient due to the overheads in managing those connections.

Websockets won't be affected by this. I'm delaying NIO specifically for reading request bodies and writing response bodies. A websocket will not take up a thread while waiting for data.

weavejester avatar Feb 29 '20 00:02 weavejester

BTW, I should also add, great job on Ring 2 and in documenting the rationale behind the decisions. It was a pleasure to read through these documents and I can't wait to use it in anger.

Thanks! I'm going to try my best to get a complete alpha out in the next two months.

weavejester avatar Feb 29 '20 00:02 weavejester

If you are already using get-in, it’s still simple:

(get-in request [:headers "content-type" 0])

Yes, that's true.

weavejester avatar Feb 29 '20 00:02 weavejester

I have a question, according to the ring spec, ring does nothing about how to create the thread that handle the request. And almost every web server library(except aleph) does not depends on async library neither.

So is it possible to use threads in core.async thread pool to handle the request?

DogLooksGood avatar Feb 29 '20 05:02 DogLooksGood

@weavejester you're right, I was not paying enough attention.

I am however a little concerned about the send-message api. Is it sync or async, what happens if you have a slow consumer, what's the backpressure story?

SevereOverfl0w avatar Feb 29 '20 09:02 SevereOverfl0w

So is it possible to use threads in core.async thread pool to handle the request?

It's possible, but that's up to the adapter.

weavejester avatar Feb 29 '20 16:02 weavejester

How about async streaming API - if my handler wants to send chunks?

That was added back in Ring 1.6, with the caveat that I/O is blocking. In other words, while your handler is actively writing data to the client it's blocking the thread.

The problem with fully supporting non-blocking I/O in Ring 1 was that the request body was an InputStream, which is a blocking stream. In Ring 2, that's been changed, and in Ring 2.1 the plan is to have NIO protocols that the request body and response body can optionally satisfy.

For Ring 2.0 I thought that sort of performance optimisation was a little out of scope, as it's a fairly niche requirement.

We use a lot a context map (with bunch of deps like db connection etc) as a first parameter to handlers - may it be useful for ring?

In general I think it's generally better to use closures for that:

(fn [context]
  (fn [request]
    ...))

But I'd be interested in hearing about use-cases where closures are not as elegant. That said, I don't anticipate changing the request/response maps into context maps instead.

Closures cons:

  • unaesthetic stack-trace;
  • Implicit state - not so easy to test or change on fly.
  • Providing request as part of context can simplify response middleware.
  • Isn't the first-order function always better than high-order? :)
  • Most handlers and mw should be wrapped this way

The idea of external chain executer instead of wrapping functions as well worth discussion. What do you think in general about Pedestal interfaces?

niquola avatar Feb 29 '20 17:02 niquola

I am however a little concerned about the send-message api. Is it sync or async, what happens if you have a slow consumer, what's the backpressure story?

Those are excellent points. Ring is designed to try to be adapted to the widest range of libraries, which often means considering the lowest common denominator. In this case the design is a synchronous send, and backpressure and buffering concerns handled by the adapter.

However, your comment has made me reconsider the design. Though not every existing Java and Clojure websocket implementation supports asynchronous sending, the majority of them do, and the key advantage of an asynchronous send is that developers can implement bespoke backpressure handling.

I'm considering adding an optional protocol like:

(defprotocol AsyncSocket
  (async-send [socket message callback]))

Or possibly just extending the current protocol. This will require a little hammock time, so feel free to suggest designs or raise further concerns in the meantime.

weavejester avatar Feb 29 '20 17:02 weavejester

Closures cons:

  • unaesthetic stack-trace;

Can you give an example?

  • Implicit state - not so easy to test or change on fly.

I'm not sure what you mean by this. Why does adding another lexical scope make things more implicit or harder to test?

  • Providing request as part of context can simplify response middleware.

How so?

  • Isn't the first-order function always better than high-order? :)

Why?

  • Most handlers and mw should be wrapped this way

I can see handlers often requiring context, such as a database connection, but I've rarely needed that for middleware outside of authorization middleware. Could you give some examples of other middleware that would require a context?

weavejester avatar Feb 29 '20 17:02 weavejester