-
Notifications
You must be signed in to change notification settings - Fork 520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ring 2.0 feedback #393
Comments
|
It adds a dependency on core.async.
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.
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. |
Yes, good catch. Let me fix that. |
So can we consider a new protocol for async API? Then we can easily bridge any async lib into ring protocol.
Both (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]))
ring can define |
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?
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.
The 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. |
How about async streaming API - if my handler wants to send chunks? |
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? |
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. |
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 For Ring 2.0 I thought that sort of performance optimisation was a little out of scope, as it's a fairly niche requirement.
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. |
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. |
If you are already using
|
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. |
Great spec, all changes very solid and timely! |
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. |
Is it required for the spec implementor (a web server) to validate this?
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. |
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. |
I'll consider adding a note explaining the terminology. |
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. |
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.
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.
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. |
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. |
Thanks! I'm going to try my best to get a complete alpha out in the next two months. |
Yes, that's true. |
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? |
@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? |
It's possible, but that's up to the adapter. |
Closures cons:
The idea of external chain executer instead of wrapping functions as well worth discussion. |
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. |
Can you give an example?
I'm not sure what you mean by this. Why does adding another lexical scope make things more implicit or harder to test?
How so?
Why?
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? |
Silly question: what's the motivation behind using a protocol for defining websocket listeners? |
Also, are we planning to support ping/pong frame handlers? |
It allows for more flexibility than, say, a map of functions. For example, you could respond with a core.async channel.
Not initially; I wanted to keep the core interface minimal to provide the greatest width of base API support. |
Thanks for the quick reply! Regarding ping/pong support, that would be a huge win for people who Edit: The graphql-ws protocol doesn't rely on ping/pong control frames. Instead it defines specific ping/pong messages for application-specific purposes typically on the frontend. Anyway, it's not uncommon to have some kind of keep-alive loop at the backend that rely on ping/pong control frames, as is our case here at Beacon. |
So to be clear, these frames would be supported by the backend, insofar as a ping from the client would automatically result in a pong from the server. They just wouldn't be accessible to the developer through the Ring protocol, at least at initial release. |
According to https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.2, ping frames are allowed to carry application data. It would be nice to have access to ping and its body. |
Sure; nice to have, but not essential. I'm not saying we shouldn't have this, just that it shouldn't be a day 1 feature. It's not part of a minimal viable interface for websockets. |
hello @weavejester What is the status of this? Can you rebase the 2.0 branch with master? We have plans to integrate the ring2 support in our adapter for testing purposes and would be awesome knowing the status of this and having it rebased on master or at least on top of latest stable release. Thanks |
Hi @niwinz. I'm afraid other obligations on my time have delayed Ring 2.0. However, I can see about squeezing some time in to merge |
I find the time I will try to prepare the merge for you. :D |
@niwinz |
thanks a lot, unfortunately as you can see I didn't find time to do it myself :( |
@weavejester Is it possible to push a new alpha release to clojars? I would like to do some testing and there are quite some significant changes since the previous alpha release (details). Thank you in advance. (FYI, I tried to create a fork with a |
Why the keys in That could further simplify working with headers. For example:
The only reason I can think of for keeping header keys as strings is some kind of runtime performance gain, since it would require a Would the saving on omitting such extra keyword conversion step worth the trade-off for the degraded developer experience? As Rich points out in https://clojure.org/about/spec#_global_namespaced_names_are_more_important
In the case of Similarly, we are using the namespace information in https://github.com/ring-clojure/ring-codec/blob/master/src/ring/util/codec.clj#L104 too, which affects form data and query params, when using All these little frictions add up to frustration and gets in the way of delivering on Clojure's core value proposition of working with fully-qualified names. Such frustrations amounted my colleagues proposing NOT to use fully-qualified keys, because the various tools and libraries are not very compatible with them. Encouraging their usage, by making certain choices at such low layers as the Ring spec, might have an amplified effect on the eco-system and at least in the Clojure parts of our code-bases we wouldn't need to fight against the non-qualified world of computing. :) Alternatively, we can agree on some kind of an extension to Ring 2.0, which lifts the most common header fields up, into the top of the request map, under fully qualified keys, which then could be even validated/generated with {:ring.request.header/accept {:internet.media/type :application/json ...}
:ring.request.header/content-type {:internet.media/type :application/json ...}}
{:ring.response.header/content-type {:internet.media/type :application/json ...}} That way we can decouple the problem of how do we parse the various, more complicated header values. |
Because they come from the client, rather than something the developer has control over. The developer might not necessarily want to intern every header, and not every valid header name is a valid keyword or symbol name. |
I've wondered about exactly this lately while adding HTTP/2 to Aleph. If you block until you are completely done reading, it's not such an issue, but it doesn't play nicely with streaming scenarios where you want to start once the headers are done, and process body data as it streams in. I haven't put too much thought into it yet, but adding a |
From SPEC-2.md:
This is not technically true for OPTIONS and CONNECT methods: https://www.rfc-editor.org/rfc/rfc9113.html#section-8.3.1-2.4.1 The new Ring ADR talks about the path in relation to OPTIONS and CONNECT, and it's correct that CONNECT will have no
Also, I think more people will read the SPEC than the ADR, so maybe it's clearer to mention there that |
I'm not sure how many users will actually use this key, but I think we should clarify the version number. Colloquially, since HTTP/2 hasn't had a minor version number, and HTTP/2 frames carry no version info, most people refer to it just "HTTP/2". But the RFC says that implicitly, the version number should be "2.0". I think we should specify it'll be "HTTP/2.0" so there's no confusion. |
Right, but the path will always be |
Sure, that works, but I think the Ring SPEC should be more precise on these points. Right now, it seems like the ADR is more detailed than the SPEC itself, but I'm pretty sure more people will refer to the SPEC once published. Maybe something like:
|
That's a reasonable point. It might also be worth allowing a path of |
It's true we couldn't consider the path as always a path in that case. Hopefully, anyone responding to OPTIONS knows what they're doing, though. Luckily, it wouldn't affect the bread-and-butter GET/POST/etc requests either way. Most users will probably never notice. |
@weavejester Think we could maybe include WebTransports in the Ring 2 spec? WebSockets have the problem that all the servers already have their own ad hoc implementation; specifying how to use WebTransports could avoid the same issue, since I don't think any Clojure server's implemented them yet. |
The Ring 2.0 specification was a redesign of the current request/response maps, along with support for new features such as WebSockets. This proved to be too ambitious, so instead new features will be added piecemeal. In Ring 1.11.0, for example, WebSocket support is going to be added. In 1.12.0 I plan on adding support for HTTP/2 to the Jetty adapter, along with some other changes, so it'll be a while before HTTP/3, and by extension WebTransports, will be supported. However, we can certainly start thinking about the design of them, and to collaborate with third-party adapters to present a more common approach from day 1. I've also been working with several third-party adapters, both to get feedback on the WebSocket API, and to help with the implementation. After Ring 1.11.0 is released, it shouldn't be long before rja9, httpkit, and the Luminus undertow adapter also support Ring's WebSocket API, which hopefully more to follow. |
Did HTTP/2 make it into 1.12.0? |
No, I decided that it was better to release sooner rather than delay 1.12. You can always check the changelog to see what's in any particular Ring release. |
I did check indeed, was just wondering, Thanks for the update! |
@LouDnl A lot of HTTP/2 is invisible or backwards-compatible at the Ring level. For 97% of use cases, the Ring map would look practically the same. Are there HTTP/2 or 3 features you want exposed in Ring? |
@KingMob no exact features, I'm just reading up on and experimenting with sse, websockets etc. |
@LouDnl 2-6 connections wasn't a limitation of HTTP/1, but a limitation imposed by browsers. HTTP/1 itself is technically only limited by the number of TCP ports, so your computer couldn't have more than 2^16=65536 connections to one server. More realistically, limits of that era were things like per-conn overhead, link bandwidth saturation, etc. With HTTP/2 streams, you can multiplex a single TCP connection up to 2^31 streams. You have your work cut out for you to actually use up all of those. If you want to know more about HTTP/2, check out RFC 9113, not RFC 9110. 9110 is more about the HTTP protocol semantics (e.g., what does GET mean, what's 204 mean? etc), which hasn't changed as much, while 9113 is more about the actual H2 framing, streams, connections, etc. |
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:
InputStream
to protocolNote 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.
The text was updated successfully, but these errors were encountered: