Re: [boost] [asio-users] [http] Formal review of Boost.Http
On Sat, Aug 8, 2015 at 5:16 AM, Vinícius dos Santos Oliveira < vinipsmaker@users.sf.net> wrote:
2015-08-08 0:10 GMT-03:00 Lee Clagett
: FWIW it should be possible to do client and server side pipelining with this design right now. The reads and writes of the HTTP Socket concept should be (and currently are by `basic_socket`) handled independently. Since the notification for a write completion is based on the lower layer write completion, and NOT whether a HTTP response was received, multiple (pipelined) requests should be possible by a client. Similarly, this design should allow servers to handle pipelined requests in parrallel. Providing a framework for client or server pipelining will require more work, and should probably not be a part of the Socket concept. In fact -
The current design **does** support pipelining. Some HTTP clients disable pipelining because some buggy servers will get confuse. This won't happen with Boost.Http (there are tests to ensure pipelning is working).
What the current design does **not** support is handling pipelined requests concurrently. When you starting read a request, the write_state[1] will change to finished until the fully request is received, so you can't get multiple request while you don't issue the replies. This behaviour is explained in the ServerSocket concept[2].
There are just too many ways to allow HTTP concurrent pipeline that would be transparent for the user, but all of them are heavier. If I was to allow handling multiple concurrent HTTP pipelined requests, I'd expose user cooperation, so the abstraction doesn't become so heavy. The user would need to be aware if the reply for some request can already be delivered or not. Of course this means the socket will need to remember order and attach some id to the request messages. It can be problematic in alternative backends because the id could be of a different type and makes it difficult to use the same message type with different HTTP backends. What do you think? The design would just be more complex for not much gain. HTTP/2.0 allow real multiplexing and doesn't show this problem.
I agree that providing any pipelining framework in the library is probably unnecessary right now. However, the design should not inhibit such functionality from being added (which I do not think is a problem with this design).
I think the ServerSocket concept should be removed entirely. The
`http::basic_socket<...>` models both concepts, so theres lots of server specific code in it, which seems confusing to me (is this really a "basic" socket?).
basic_socket uses basic_ prefix just like basic_string.
I was asking whether basic_socket should NOT model the http::ServerSocket concept. I understood the naming convention, and the decision to provide the implementation as a configurable template. The problem is the lack of composability. If a different http::Socket concept is desired (someone not using ASIO, etc.), then the http::ServerSocket concept has to be re-implemented also since there is no other implementation. The obvious way is to achieve composability is to use inheritance with: `http::basic_server_socket<Socket>` where `Socket` is a http::Socket concept. Inheritance in this situation has its drawbacks, for sure (a virtual destructor should be considered).
I think standalone functions for typical client and server operations can
be implemented as "composed" operations similar to `asio::async_write`. The documentation will have to be explicit on what the composed function is doing so that users know whether the Socket concept has outstanding reads or writes (and therefore cannot do certain operations until the handler is invoked). In fact, if there is a server operation that _cannot_ be done with the Socket concept, then the concept probably needs to be re-designed.
About "users know whether..." seems like a bad idea if I want to deliver multiple backends. Some implementation details should just be abstracted away.
http::SocketServer implementations manipulate the state of the http::Socket (and indirectly the asio::tcp::socket), making some actions unavailable after invoking those functions. For example, after invoking `async_write_response`, no writes on the http::Socket/http::ServerSocket or asio::ip::tcp::socket can occur until the callback is invoked because it calls asio::write on the asio::ip::tcp::socket directly. So unless I am incorrect, it is important for the users to know whats being manipulated (the effects).
I don't understand what you mean by "if there is a server operation that
cannot be done with the Socket concept [...]". Boost.Http has two concepts, ServerSocket and Socket. Socket concept will be useful to client-side HTTP and that's the reason why there are two concepts.
The http::ServerSocket concept requires a http::Socket to write its messages, but does the http::ServerSocket concept need to be a refinement of the http::Socket concept? This implies a strong relationship. The current specification looks like a http::ServerTraits concept - its specifying how a http::Socket is being used in situations specific to servers. There doesn't appear to be any additional state tracking needed for the http::ServerSocket functions, which is why I suggested standalone functions (I didn't see the FileServer section). In fact implementation can be inverted; make all of the current implementations for http::ServerSocket standalone functions, and then have the default http::ServerTraits implementation call the standalone versions of the functions. The http::ServerTraits should then take a http::Socket as an argument for each of the functions. This complete de-coupling would allow someone to independently change the behavior of any related server functions, even while using the same http::Socket object. [1] https://boostgsoc14.github.io/boost.http/reference/write_state.html
[2] "The ServerSocket object MUST prevent the user from issuing new replies while [...]", https://boostgsoc14.github.io/boost.http/reference/server_socket_concept.htm...
Lee
2015-08-08 16:36 GMT-03:00 Lee Clagett
[...] The problem is the lack of composability. If a different http::Socket concept is desired (someone not using ASIO, etc.), then the http::ServerSocket concept has to be re-implemented also since there is no other implementation.
Exactly. The obvious way is to achieve composability is to use inheritance with:
`http::basic_server_socket<Socket>` where `Socket` is a http::Socket concept. Inheritance in this situation has its drawbacks, for sure (a virtual destructor should be considered).
I don't know. The only place that I see that can be abstracted among all alternative http backends is management of read_state and write_state and some auxiliary functions to check user intent (close connection...). The auxiliary functions can be provided without a base class and the read_state and write_state code is too little to convince me it's worth. http::SocketServer implementations manipulate the state of the http::Socket
(and indirectly the asio::tcp::socket), making some actions unavailable after invoking those functions. For example, after invoking `async_write_response`, no writes on the http::Socket/http::ServerSocket or asio::ip::tcp::socket can occur until the callback is invoked because it calls asio::write on the asio::ip::tcp::socket directly. So unless I am incorrect, it is important for the users to know whats being manipulated (the effects).
The documentation points the use of composed operations: https://boostgsoc14.github.io/boost.http/reference/basic_socket.html It's akin to Asio documentation on composed operations: http://www.boost.org/doc/libs/1_58_0/doc/html/boost_asio/reference/async_wri... (i.e. "the stream performs no other [...] until this operation completes") But now that you mentioned, I see that documentation can receive some improvements on the topic. The ServerSocket concept doesn't mention that some models can make use of this restriction nor there is any trait to detect if a model makes use of composed operations. The http::ServerSocket concept requires a http::Socket to write its
messages, but does the http::ServerSocket concept need to be a refinement of the http::Socket concept? This implies a strong relationship. The current specification looks like a http::ServerTraits concept - its specifying how a http::Socket is being used in situations specific to servers. There doesn't appear to be any additional state tracking needed for the http::ServerSocket functions, which is why I suggested standalone functions (I didn't see the FileServer section). In fact implementation can be inverted; make all of the current implementations for http::ServerSocket standalone functions, and then have the default http::ServerTraits implementation call the standalone versions of the functions. The http::ServerTraits should then take a http::Socket as an argument for each of the functions. This complete de-coupling would allow someone to independently change the behavior of any related server functions, even while using the same http::Socket object.
If I understood you correctly, you're proposing that all functions should be free functions, not member-functions, then everybody can customize any type to model a ServerSocket. And then you criticize the strong relationship between Socket and ServerSocket. Asio doesn't follow this model (async_read_some is a member-function, not a free function). It does appear to be an interesting idea, but I'm not sure I'm prepared to solve this detail of customization level. The order of includes could completely change the behaviour and this is the least of the problems. Can you elaborate more, then we can discuss? And the reason behind Socket and ServerSocket is the future addition of a ClientSocket, which will be a refinement of Socket. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On Sun, Aug 9, 2015 at 6:24 AM, Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com> wrote:
2015-08-08 16:36 GMT-03:00 Lee Clagett
: [...] The problem is the lack of composability. If a different http::Socket concept is desired (someone not using ASIO, etc.), then the http::ServerSocket concept has to be re-implemented also since there is no other implementation.
Exactly.
The obvious way is to achieve composability is to use inheritance with:
`http::basic_server_socket<Socket>` where `Socket` is a http::Socket concept. Inheritance in this situation has its drawbacks, for sure (a virtual destructor should be considered).
I don't know. The only place that I see that can be abstracted among all alternative http backends is management of read_state and write_state and some auxiliary functions to check user intent (close connection...). The auxiliary functions can be provided without a base class and the read_state and write_state code is too little to convince me it's worth.
http::SocketServer implementations manipulate the state of the http::Socket
(and indirectly the asio::tcp::socket), making some actions unavailable after invoking those functions. For example, after invoking `async_write_response`, no writes on the http::Socket/http::ServerSocket or asio::ip::tcp::socket can occur until the callback is invoked because it calls asio::write on the asio::ip::tcp::socket directly. So unless I am incorrect, it is important for the users to know whats being manipulated (the effects).
The documentation points the use of composed operations: https://boostgsoc14.github.io/boost.http/reference/basic_socket.html
It's akin to Asio documentation on composed operations:
http://www.boost.org/doc/libs/1_58_0/doc/html/boost_asio/reference/async_wri... (i.e. "the stream performs no other [...] until this operation completes")
But now that you mentioned, I see that documentation can receive some improvements on the topic. The ServerSocket concept doesn't mention that some models can make use of this restriction nor there is any trait to detect if a model makes use of composed operations.
The http::ServerSocket concept requires a http::Socket to write its
messages, but does the http::ServerSocket concept need to be a refinement of the http::Socket concept? This implies a strong relationship. The current specification looks like a http::ServerTraits concept - its specifying how a http::Socket is being used in situations specific to servers. There doesn't appear to be any additional state tracking needed for the http::ServerSocket functions, which is why I suggested standalone functions (I didn't see the FileServer section). In fact implementation can be inverted; make all of the current implementations for http::ServerSocket standalone functions, and then have the default http::ServerTraits implementation call the standalone versions of the functions. The http::ServerTraits should then take a http::Socket as an argument for each of the functions. This complete de-coupling would allow someone to independently change the behavior of any related server functions, even while using the same http::Socket object.
If I understood you correctly, you're proposing that all functions should be free functions, not member-functions, then everybody can customize any type to model a ServerSocket. And then you criticize the strong relationship between Socket and ServerSocket.
Asio doesn't follow this model (async_read_some is a member-function, not a free function). It does appear to be an interesting idea, but I'm not sure I'm prepared to solve this detail of customization level. The order of includes could completely change the behaviour and this is the least of the problems. Can you elaborate more, then we can discuss?
And the reason behind Socket and ServerSocket is the future addition of a ClientSocket, which will be a refinement of Socket.
The problem is that I incorrectly jumped to conclusions after seeing the http::Socket concept, and assumed it had all the functionality to parse HTTP (server and client), because thats exactly how I would try do it for flexibility purposes. This http::Socket concept explicitly does not provide a mechanism to read the first line & headers of a HTTP message. I expected the concept to handle the fields portion of the header at least, since they are identical. This is why I thought the http::ServerSocket were more like traits - I thought they were defining some convenience functions into the more basic calls of the http::Socket concept. Anyway- Can http::Socket concept define a function for reading the headers (they should be identical)? OR can the first line of a http message be read into the empty "" string of the headers map? OR could the http::Socket require a separate string for the first line? If either of the last two were chosen - standalone functions could "compose" against a http::Socket::async_read_headers function, where upon completion they would move the first line to some place else or parse them apart them into separate fields provided by the user. The major funky thing is handling HEAD command responses, which I assume was going to have a specific function in http::ClientSocket ? This might need a special state manipulation regardless. I'll probably have to start using it before saying much more I guess ... Lee
2015-08-09 17:46 GMT-03:00 Lee Clagett
Can http::Socket concept define a function for reading the headers (they should be identical)?
It can, but it'd make the design more complex. I want to keep the design simple, so users won't get confused. If OR can the first line of a http message be read into
the empty "" string of the headers map?
It's a clever idea. Too clever, indeed. And you end up losing all your type safety. The level of cleverness that I don't appreciate. OR could the http::Socket require a
separate string for the first line?
It has the same problem of the lack of type safety. A boost::variant could be used to partially solve the problem, but the wrong type still could be passed around. If either of the last two were chosen - standalone functions could
"compose" against a http::Socket::async_read_headers function, where upon completion they would move the first line to some place else or parse them apart them into separate fields provided by the user. The major funky thing is handling HEAD command responses, which I assume was going to have a specific function in http::ClientSocket ? This might need a special state manipulation regardless.
The major problem I see is the added complexity. There would be more values for read_state and write_state (like first_line_read, headers_read). I don't see a major benefit. The separate states for the body has a clear use case (not exhaust memory and allow live video stream and stuff). I'd like to read/hear opinion on other members of the list if they appreciate the added complexity and think this is the right decision. I can provide any, maybe we can discuss. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
participants (2)
-
Lee Clagett
-
Vinícius dos Santos Oliveira