Extend HTTP server to handle more scenarios #14234

jagannatharjun · 2021-01-17T09:27:36Z

Untie IRequestHandler and Connection class. Make Interface of IRequestHandler more barebones, and implement existing behavior of IRequesteHandler class in new class BasicRequestHandler (implements IRequestHandler). Make Connection class directly handle serializing of HTTP responses and add new functions to the class accordingly.

Obsolete implementation summary

Untie IRequestHandler and Connection classes. Have Server takes a new interface IConnectionHandler which only takes a Connection do as it wishes, Base IRequestHandler (renamed to RequestHandler) on IConnectionHandler that provides more user-friendly interface to work with. Also, add some more convenient functions to the Connection class to facilitate sending HTTP responses

Extends HTTP server by introducing two new classes HttpSocket, IHttpConnectionHandler. renaming IRequestHandler => RequestHandler. Previous class Connection and IRequestHandler were very closely tied making types of requests that can be served very small. Now Http::Server instantiate a HttpSocket with for every new connection, HttpSocket will parse the request and emit a signal, Server will catch this signal and pass the HttpSocket to IHttpConnectionhandler, The interface of IHttpConnectionhandler is fundamental. Thus class RequestHandler class is provided that transforms the IHttpConnectionHandler interface to IRequestHandler

Required for #12542

src/base/http/httpsocket.h

src/base/http/connection.cpp

glassez · 2021-01-18T15:19:26Z

src/base/http/requesthandler.h

        virtual Response processRequest(const Request &request, const Environment &env) = 0;
+
+    private:
+        void handleConnection(Connection *socket) override final;


final implies override so it's redundant to use both.

Maybe Connection *connection?

And what's the point to redeclare it private? It implements interface method that's public by definition.

And what's the point to redeclare it private? It implements interface method that's public by definition.

At some point in time, I was instructed to do something similar in another PR.

And what's the point to redeclare it private? It implements interface method that's public by definition.

At some point in time, I was instructed to do something similar in another PR.

Are you sure it was about exactly the same case? Maybe it was about the case when you override previously protected method in final class?

final implies override so it's redundant to use both.

No it doesn't: https://stackoverflow.com/questions/29412412/does-final-imply-override

No it doesn't: https://stackoverflow.com/questions/29412412/does-final-imply-override

In such case it does (defacto). final requires the method to be virtual. But since this declaration does not have virtual specifier, it can only be virtual if it is overriden. I would prefer to use minimal set of specifiers in method declarations. Or does it seem less readable to you?

I was taking your comment at face value and didn't read the code. But if that particular function isn't virtual then it cannot be override and/or final anyway. It would be a compile error. Or are you saying something else?

Scratch whatever I said. I missed your point and the rest of the SO answer. Yes, final override is superfluous. Because final requires a virtual function aka something that is overridable (sic). In that case override won't achieve something more. Maybe only a clearer intent indication to the reader. But let's not go there.

glassez · 2021-01-18T15:22:37Z

src/base/http/requesthandler.cpp

+#include "connection.h"
+#include "httputils.h"
+
+using namespace Http;


Why this using statement if you still use fully qualified names below?

src/base/http/requesthandler.h

glassez · 2021-01-18T17:37:05Z

src/base/http/server.cpp

+    connect(socket, &Connection::disconnected, this, [socket, this]() { removeConnection(socket); });
+    connect(socket, &Connection::readyRequest, this, [socket, this]()
+    {
+        m_connectionHandler->handleConnection(socket);


Seems it handles not Connection itself but each request received within Connection.

hmm, then I will have to use QObject::Connect in non QObject classes, I don't think you would like it either.

src/base/http/connection.h

glassez · 2021-01-18T18:34:58Z

src/base/http/connection.h


-        bool hasExpired(qint64 timeout) const;
-        bool isClosed() const;
+        void sendStatus(const Http::ResponseStatus &status);


You don't need to use fully qualified names within the same namespace.

src/base/http/httputils.cpp

src/base/http/server.cpp

src/base/http/connection.cpp

glassez · 2021-01-19T09:12:31Z

src/base/http/requesthandler.cpp

+
+void Http::RequestHandler::handleConnection(Connection *socket)
+{
+    Q_ASSERT(socket->status() == Connection::PendingStatus);


It's a really bad idea to have such assertion in interface method.
If it handles Connection it shouldn't restrict the state (or other properties) of this connection to be accepted as input parameter.
As I said before it doesn't handle Connection currently. It simply handles each specific request received within the Connection, but in a poorly designed form that hides the essence of things.

If you really need "new interface IConnectionHandler which only takes a Connection do as it wishes" you should pass Connection once it is constructed and allow handler to do what it wants further.
Otherwise you need other handler interface that allows correct per-request handling.

ok, I agree with your comments about the interface, the problem is that I don't want to make RequestHandler a QObject. or since now it's not a pure interface maybe I can make it a QObject.

maybe I can make it a QObject.

Maybe...
Can you describe how your another ConnectionHandler should interacts with Connection?

this is based on an older version of this PR jagannatharjun@76002fc#diff-877ca73ad30842b71bcdadd27bac68ce563ed8aa9af32a65c667ad08b8de4d9aR155

this is based on an older version of this PR jagannatharjun/qBittorrent@76002fc#diff-877ca73ad30842b71bcdadd27bac68ce563ed8aa9af32a65c667ad08b8de4d9aR155

Well, let's try to figure out what the differences are between the existing handlers (web application and tracker) and the one that is needed to support media streaming.

Existing implementation just handles each request and send response. It doesn't close connection after each response. Connection can be closed either by client side or by timeout (and maybe due to underlying network error). It's "request handling" in its essence so it can be easily implemented without direct access to connection. When request is received it is passed to the handler, who processes it and returns a response that is sent to the client (i.e. all network IO can be done outside of handler).

Another handler still "just handles each request and send response" as existing one. The differences (judging by the code above) are that:

It closes connection after each request;

It handles request asynchronously.

As for (1), I have a question, is this really required, or can it re-use the existing connection to send multiple requests?
Anyway the connection can be closed outside the handler based on some Response attribute. So the main problem is (2). Even if writing to the socket in parts, mentioned by @Chocobo1, turns out to be a bad idea, and we stop at accumulating the entire Response, and only then sending it to the client, it will still not be available immediately. Apparently, here it remains only to use the Qt meta-object system, and allow the handler to return the generated Response not directly from the method, but through the signal. You only need to establish some kind of relationship between the Request, its Connection, and the Response.

While I was thinking about all this and writing the previous long comment, I came up with the idea of a compromise option.
You just add a property to the Request that contains a pointer to its Connection. When the next Request is received, you pass it to the handler, so that it will still be the RequestHandler, and then it is responsible for processing the Request and sending the response through the Connection.
I.e., you do not have to change almost anything. Only rename IConnectionHandler to IRequestHandler, RequestHandler to something like BasicRequestHandler. Also you need to obtain the Request instance outside of the handler and pass it through the IRequestHandler::handleRequest() method.
That's all, if I didn't miss anything important.

You just add a property to the Request that contains a pointer to its Connection.

IMO making it property will expose connection to too many places. so I am providing connection as an argument in IRequestHander::handleRequest.

making it property will expose connection to too many places.

What exactly are the places that bother you? In any case, you expose it to the outside world (even through the parameter). Do you want to hide it from the inner space, where it also comes from?

What exactly are the places that bother you?

BasicRequestHandler::processReqeust tryCompressContent.

In any case, you expose it to the outside world (even through the parameter)

with parameter it is only exposed to IrequestHandler::handleRequest and doesn't leak to outside space.

with parameter it is only exposed to IrequestHandler::handleRequest and doesn't leak to outside space.

...unless you allow it in IRequestHandler::handleRequest().
Well, let's keep it that way for now.

glassez · 2021-01-24T11:08:33Z

src/base/http/basicrequesthandler.h

+    public:
+        virtual ~BasicRequestHandler() = default;
+
+        virtual Response processRequest(const Request &request, const Environment &env) = 0;


Seems now it is only intended to be implemented in subclasses. It should be used only inside this class (as an implementation detail of handleRequest()), isn't it?

src/base/http/basicrequesthandler.h

glassez · 2021-01-24T11:20:46Z

src/base/http/server.cpp

@@ -108,9 +109,13 @@ void Server::incomingConnection(const qintptr socketDescriptor)
        static_cast<QSslSocket *>(serverSocket)->startServerEncryption();
    }

-    auto *c = new Connection(serverSocket, m_requestHandler, this);


So why not keep this logic?

IMO it's cleaner to keep these two classes separated.

src/base/http/connection.h

glassez · 2021-01-24T11:37:04Z

src/base/http/basicrequesthandler.cpp

+    connection->sendStatus(response.status);
+    connection->sendHeaders(response.headers);
+    if (!response.content.isEmpty())
+        connection->sendContent(response.content);


So why not serialize/write response at once (as it was before)?

in #12542 I send content in chunks, so to support that I've added sendHeaders and sendContent functions in Connection class. I can add sendResponse function in the Connection class but let's keep Response handling in this class.

let's keep Response handling in this class.

What's the reason? Connection acts on HTTP layer. It parses received HTTP requests into Request object so why it shouldn't make an opposite action, i.e. serialize Response object? In addition, it will give more control over its integrity and consistency.

in #12542 I send content in chunks

Is it really useful? IIRC, your tests show it is not so optimal.

let's keep Response handling in this class.

What's the reason? Connection acts on HTTP layer. It parses received HTTP requests into Request object so why it shouldn't make an opposite action, i.e. serialize Response object? In addition, it will give more control over its integrity and consistency.

connection class already provide an interface for sending data, why pollute it by providing other ways to do the same. As far as the integrity of responses is concerned. I don't want to add such logic in Connection class. I would like to keep it barebones and have the request handler handle everything.

in #12542 I send content in chunks

Is it really useful? IIRC, your tests show it is not so optimal.

in #12542 client can ask for 1 gig of file. would you like to send all that at once? with the latest revision, this PR uses a cache for sending headers and status this makes it on par with the current master in terms of performance.

in #12542 client can ask for 1 gig of file.

Are you going to accept such request?

would you like to send all that at once?

I assume you don't, but then how do you come back later after sending partial of it?

Are you going to accept such request?

yep, it's a valid request

I assume you don't, but then how do you come back later after sending partial of it?

this functionality is basically to allow https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests on downloading torrent files, I use libtorrent::read_piece which is asynchronous so there is a natural callback there. So we basically fulfill HTTP range requests in size of torrent piece length using libtorrent::read_piece. Implementation is here https://github.com/jagannatharjun/qBittorrent/blob/stream-final/src/base/streaming/streamfile.cpp#L144

I use libtorrent::read_piece which is asynchronous so there is a natural callback there.

Excuse me I haven't got time to check your code, so how do you handle libtorrent::read_piece() alert that will return in unspecified/random order? Do you issue only one read_piece() at a time?

this functionality is basically to allow https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests

Some more questions, which of the following do you intend to support?

Accept-Ranges: bytes (this is the minimum requirement right?)

Accept-Ranges: none

Single part ranges

Multipart ranges

Conditional range requests

I hope we can aim to support the smallest set that will satisfy your need.

how do you handle libtorrent::read_piece() alert that will return in unspecified/random order? Do you issue only one read_piece() at a time?

yes, read_piece() is issued one at a time.

which of the following do you intend to support?

only Single Part ranges.

src/base/http/basicrequesthandler.cpp

src/base/http/connection.h

Chocobo1 · 2021-01-26T04:26:07Z

src/base/http/connection.h

-        bool isClosed() const;
+        void sendStatus(const ResponseStatus &status);
+        void sendHeaders(const HeaderMap &headers);
+        void sendContent(const QByteArray &data);


You didn't really send out anything by calling them, add*** would be perhaps more correct.

Can't we see it's not sending content immediately as an implementation detail. I mean Python's HTTP server also has the same functions and they too don't send the data immediately. add*** in Connection doesn't seem right.

Let me explain what I would expect:

Response res; // a specific class for building response res.setStatus(...); res.setHeaders(...); res.setContent(...); connection.send(res); // really send it out from socket

The current way of send* functions are not intuitive for general use and couldn't be more wrong as you are mixing higher layer data (http) with low level socket operations in the same class.

EDIT: ~~please look up OSI model if you haven't already.~~ I meant this: https://en.wikipedia.org/wiki/Communication_protocol#Software_layering

IIRC, his main reason is the need to send very large data (several gigabytes) in a single HTTP response.

I aimed to implement something like Python Http Handler https://docs.python.org/3/library/http.server.html#http.server.BaseHTTPRequestHandler.send_header
it's used by a large audience so IMO there is nothing wrong with this PR's abstractions. I am open to ideas.

To clarify once again (if this was unclear from my previous comments), I suggest having two types of Responses, one for general cases and the other for the case of sequential write content.

Agree, I think your suggestion really make the interface robust enough to avoid (most) misuse and would still preserve the correct layering.

#14234 (comment)

In the simplified example, I want to note that it might need checking if the socket is still open before sending data.

may be related to single responsibility principal.

If you talk about Http::Connection it is still responsible for sending/receiving HTTP "packets" (requests/responses).

your solution creates two branches in the behavior of a concrete class.

In fact, you can use the second one (#14234 (comment)) in all cases.

HTTP responses are well defined.

Yes.
I agree that it is possible to have the simplest interface with three methods (as you suggest) and make the correctness of their use the responsibility of the user of the class. And this should not even cause any problems in the simplest case, when you call them in a row in one place. But how to control the correctness of the logic in more complex cases (for example, in what you intend to implement next), where the calls of these methods are located far from each other (both in code and at runtime)? How are you going to check the correctness of your logic? Just inserting assertions and waiting for them to hit? Assertions is a good tool that should not be neglected, because we often can not predict all the mistakes. But why not add to it an interface that eliminates the possibility of making such mistakes (at least some of the possible ones)?

ok now it make sense.

How about this -

class ResponseContent { public: virtual size_t size() const = 0; virtual size_t read(void *data, size_t len) = 0; signals: void readyRead(); };

and provide an implementation based on QByteArray for convenience;

How about this -

I'm sorry, it doesn't make sense to me until you show me how you're going to use it.

make Response::Content (https://github.com/qbittorrent/qBittorrent/blob/master/src/base/http/types.h#L12) a std::shared_ptr< ResponseContent >
then everything will remain as it is in the current master. I just need to change Connection::sendResponse to handle this async type operation.

src/base/http/connection.h

src/base/http/basicrequesthandler.cpp

src/base/http/connection.h

src/base/http/connection.cpp

glassez

Extend Http server to handle more scenerios

Isn't "scenerios" a mistake? I took the liberty of correcting the PR title. Please correct the commit message accordingly.

src/base/http/connection.h

jagannatharjun · 2021-12-09T06:14:56Z

@glassez @Chocobo1

would it be fine if I don't touch the existing HTTP library of qBittorrent at all for this feature and implement the proposed feature totally on its own? HTTP library of qBittorrent is not built for this feature at all.

jagannatharjun force-pushed the new-http branch 4 times, most recently from 196abe2 to 7ec80e7 Compare January 17, 2021 09:54

glassez reviewed Jan 17, 2021

View reviewed changes

src/base/http/httpsocket.h Outdated Show resolved Hide resolved

src/base/http/httpsocket.h Outdated Show resolved Hide resolved

src/base/http/httpsocket.h Outdated Show resolved Hide resolved

Chocobo1 reviewed Jan 18, 2021

View reviewed changes

src/base/http/connection.cpp Show resolved Hide resolved

jagannatharjun force-pushed the new-http branch 3 times, most recently from 5ac005b to 1bf094e Compare January 18, 2021 07:43

jagannatharjun requested review from glassez and Chocobo1 January 18, 2021 14:17

glassez reviewed Jan 18, 2021

View reviewed changes

Chocobo1 reviewed Jan 19, 2021

View reviewed changes

glassez reviewed Jan 19, 2021

View reviewed changes

glassez self-assigned this Jan 20, 2021

glassez added the Core label Jan 20, 2021

glassez changed the title ~~Extend Http server to handle more scenerios~~ Extend HTTP server to handle more scenarios Jan 20, 2021

jagannatharjun force-pushed the new-http branch 2 times, most recently from f20f5aa to b838458 Compare January 23, 2021 12:49

jagannatharjun requested review from glassez and Chocobo1 January 23, 2021 12:53

glassez reviewed Jan 24, 2021

View reviewed changes

Chocobo1 reviewed Jan 26, 2021

View reviewed changes

jagannatharjun force-pushed the new-http branch from b838458 to 7561f3e Compare January 26, 2021 12:14

glassez reviewed Jan 26, 2021

View reviewed changes

src/base/http/connection.cpp Outdated Show resolved Hide resolved

glassez reviewed Jan 27, 2021

View reviewed changes

src/base/http/connection.h Outdated Show resolved Hide resolved

jagannatharjun force-pushed the new-http branch from 7561f3e to 4688bc2 Compare January 29, 2021 07:03

Extend Http server to handle more scenarios

8e474ad

jagannatharjun force-pushed the new-http branch from 4688bc2 to 8e474ad Compare January 29, 2021 07:11

jagannatharjun requested review from glassez, Chocobo1 and sledgehammer999 January 29, 2021 07:11

This was referenced Feb 4, 2021

Can qbittorrent play while downloading? How can the downloaded data be sliced into hls and played #14226

Closed

Add advanced options for piece control defaults #14338

Closed

Extend HTTP server to handle more scenarios #14234

Are you sure you want to change the base?

Extend HTTP server to handle more scenarios #14234

Conversation

jagannatharjun commented Jan 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagannatharjun Jan 19, 2021 • edited by glassez

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sledgehammer999 Jan 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagannatharjun Jan 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 Jan 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagannatharjun Jan 31, 2021 • edited

Choose a reason for hiding this comment

glassez left a comment

Choose a reason for hiding this comment

jagannatharjun commented Dec 9, 2021

jagannatharjun commented Jan 17, 2021 •

edited

jagannatharjun Jan 19, 2021 •

edited by glassez

sledgehammer999 Jan 22, 2021 •

edited

jagannatharjun Jan 30, 2021 •

edited

Chocobo1 Jan 29, 2021 •

edited

jagannatharjun Jan 31, 2021 •

edited