Unverified Commit 2bfdb96b authored by Igor [hyperxor]'s avatar Igor [hyperxor] Committed by GitHub

Code improvements in HTTP Parsers (#717)

* Code improvements in HTTP Parsers

* Trigger CI
parent 86b27be5
......@@ -105,7 +105,7 @@ private:
Queue<RequestData> requestsQueue;
TimerPool timerPool_;
Private::Parser<Http::Response> parser;
ResponseParser parser;
};
class ConnectionPool {
......
......@@ -23,13 +23,15 @@
#include <pistache/http_headers.h>
#include <pistache/mime.h>
#include <pistache/net.h>
#include <pistache/peer.h>
#include <pistache/stream.h>
#include <pistache/tcp.h>
#include <pistache/transport.h>
#include <pistache/view.h>
namespace Pistache {
namespace Tcp {
class Peer;
}
namespace Http {
namespace details {
......@@ -49,7 +51,6 @@ template <typename P> struct IsHttpPrototype {
typedef Pistache::Http::details::prototype_tag tag;
namespace Private {
template <typename T> class Parser;
class RequestLineStep;
class ResponseLineStep;
class HeadersStep;
......@@ -148,7 +149,6 @@ private:
class Request : public Message {
public:
friend class Private::RequestLineStep;
friend class Private::Parser<Http::Request>;
friend class RequestBuilder;
......@@ -165,14 +165,14 @@ public:
const Uri::Query &query() const;
/* @Investigate: this is disabled because of a lock in the shared_ptr /
weak_ptr implementation of libstdc++. Under contention, we experience a
performance drop of 5x with that lock
/* @Investigate: this is disabled because of a lock in the shared_ptr /
weak_ptr implementation of libstdc++. Under contention, we experience a
performance drop of 5x with that lock
If this turns out to be a problem, we might be able to replace the
weak_ptr trick to detect peer disconnection by a plain old "observer"
pointer to a tcp connection with a "stale" state
*/
If this turns out to be a problem, we might be able to replace the
weak_ptr trick to detect peer disconnection by a plain old "observer"
pointer to a tcp connection with a "stale" state
*/
#ifdef LIBSTDCPP_SMARTPTR_LOCK_FIXME
std::shared_ptr<Tcp::Peer> peer() const;
#endif
......@@ -330,7 +330,6 @@ inline ResponseStream &operator<<(ResponseStream &stream,
class Response : public Message {
public:
friend class Private::ResponseLineStep;
friend class Private::Parser<Http::Response>;
Response() = default;
explicit Response(Version version);
......@@ -542,27 +541,31 @@ private:
StreamCursor cursor;
};
template <typename Message> class Parser;
template <typename Message> class ParserImpl;
template <> class Parser<Http::Request> : public ParserBase {
template <> class ParserImpl<Http::Request> : public ParserBase {
public:
explicit Parser(size_t maxDataSize);
explicit ParserImpl(size_t maxDataSize);
void reset() override;
Request request;
};
template <> class Parser<Http::Response> : public ParserBase {
template <> class ParserImpl<Http::Response> : public ParserBase {
public:
explicit Parser(size_t maxDataSize);
explicit ParserImpl(size_t maxDataSize);
Response response;
};
} // namespace Private
using Parser = Private::ParserBase;
using RequestParser = Private::ParserImpl<Http::Request>;
using ResponseParser = Private::ParserImpl<Http::Response>;
class Handler : public Tcp::Handler {
public:
void onInput(const char *buffer, size_t len,
......@@ -583,8 +586,7 @@ public:
virtual ~Handler() override {}
private:
Private::Parser<Http::Request> &
getParser(const std::shared_ptr<Tcp::Peer> &peer) const;
RequestParser &getParser(const std::shared_ptr<Tcp::Peer> &peer) const;
private:
size_t maxRequestSize_ = Const::DefaultMaxRequestSize;
......
......@@ -12,6 +12,7 @@
#include <unordered_map>
#include <pistache/async.h>
#include <pistache/http.h>
#include <pistache/net.h>
#include <pistache/os.h>
#include <pistache/stream.h>
......@@ -23,11 +24,6 @@
#endif /* PISTACHE_USE_SSL */
namespace Pistache {
namespace Http {
namespace Private {
class ParserBase;
}
} // namespace Http
namespace Tcp {
class Transport;
......@@ -47,12 +43,9 @@ public:
void *ssl() const;
void putData(std::string name,
std::shared_ptr<Pistache::Http::Private::ParserBase> data);
std::shared_ptr<Pistache::Http::Private::ParserBase>
getData(std::string name) const;
std::shared_ptr<Pistache::Http::Private::ParserBase>
tryGetData(std::string name) const;
void putData(std::string name, std::shared_ptr<Http::Parser> data);
std::shared_ptr<Http::Parser> getData(std::string name) const;
std::shared_ptr<Http::Parser> tryGetData(std::string name) const;
Async::Promise<ssize_t> send(const RawBuffer &buffer, int flags = 0);
......@@ -68,9 +61,7 @@ private:
Address addr;
std::string hostname_;
std::unordered_map<std::string,
std::shared_ptr<Pistache::Http::Private::ParserBase>>
data_;
std::unordered_map<std::string, std::shared_ptr<Http::Parser>> data_;
void *ssl_ = nullptr;
};
......
......@@ -797,10 +797,8 @@ Async::Promise<ssize_t> ResponseWriter::putOnWire(const char *data,
.then<std::function<Async::Promise<ssize_t>(int)>,
std::function<void(std::exception_ptr &)>>(
[=](int /*l*/) {
return Async::Promise<ssize_t>(
[=](Async::Deferred<ssize_t> /*deferred*/) mutable {
return;
});
return Async::Promise<ssize_t>([=](
Async::Deferred<ssize_t> /*deferred*/) mutable { return; });
},
[=](std::exception_ptr &eptr) {
......@@ -892,23 +890,20 @@ Async::Promise<ssize_t> serveFile(ResponseWriter &writer,
#undef OUT
}
Private::Parser<Http::Request>::Parser(size_t maxDataSize)
Private::ParserImpl<Http::Request>::ParserImpl(size_t maxDataSize)
: ParserBase(maxDataSize), request() {
allSteps[0].reset(new RequestLineStep(&request));
allSteps[1].reset(new HeadersStep(&request));
allSteps[2].reset(new BodyStep(&request));
}
void Private::Parser<Http::Request>::reset() {
void Private::ParserImpl<Http::Request>::reset() {
ParserBase::reset();
request.headers_.clear();
request.body_.clear();
request.resource_.clear();
request.query_.clear();
request = Request();
}
Private::Parser<Http::Response>::Parser(size_t maxDataSize)
Private::ParserImpl<Http::Response>::ParserImpl(size_t maxDataSize)
: ParserBase(maxDataSize), response() {
allSteps[0].reset(new ResponseLineStep(&response));
allSteps[1].reset(new HeadersStep(&response));
......@@ -963,8 +958,7 @@ void Handler::onInput(const char *buffer, size_t len,
}
void Handler::onConnection(const std::shared_ptr<Tcp::Peer> &peer) {
peer->putData(ParserData, std::make_shared<Private::Parser<Http::Request>>(
maxRequestSize_));
peer->putData(ParserData, std::make_shared<RequestParser>(maxRequestSize_));
}
void Handler::onDisconnection(const std::shared_ptr<Tcp::Peer> & /*peer*/) {}
......@@ -1005,10 +999,9 @@ void Handler::setMaxResponseSize(size_t value) { maxResponseSize_ = value; }
size_t Handler::getMaxResponseSize() const { return maxResponseSize_; }
Private::Parser<Http::Request> &
RequestParser &
Handler::getParser(const std::shared_ptr<Tcp::Peer> &peer) const {
return static_cast<Private::Parser<Http::Request> &>(
*peer->getData(ParserData));
return static_cast<RequestParser &>(*peer->getData(ParserData));
}
} // namespace Http
......
......@@ -75,8 +75,7 @@ int Peer::fd() const {
return fd_;
}
void Peer::putData(std::string name,
std::shared_ptr<Pistache::Http::Private::ParserBase> data) {
void Peer::putData(std::string name, std::shared_ptr<Http::Parser> data) {
auto it = data_.find(name);
if (it != std::end(data_)) {
throw std::runtime_error("The data already exists");
......@@ -85,8 +84,7 @@ void Peer::putData(std::string name,
data_.insert(std::make_pair(std::move(name), std::move(data)));
}
std::shared_ptr<Pistache::Http::Private::ParserBase>
Peer::getData(std::string name) const {
std::shared_ptr<Http::Parser> Peer::getData(std::string name) const {
auto data = tryGetData(std::move(name));
if (data == nullptr) {
throw std::runtime_error("The data does not exist");
......@@ -95,8 +93,7 @@ Peer::getData(std::string name) const {
return data;
}
std::shared_ptr<Pistache::Http::Private::ParserBase>
Peer::tryGetData(std::string(name)) const {
std::shared_ptr<Http::Parser> Peer::tryGetData(std::string(name)) const {
auto it = data_.find(name);
if (it == std::end(data_))
return nullptr;
......
......@@ -11,7 +11,7 @@ using namespace Pistache;
// @Todo: Add an easy to use fixture to inject data for parsing tests.
TEST(http_parsing_test, should_parse_http_request_in_two_packets_issue_160) {
Http::Private::Parser<Http::Request> parser(Const::DefaultMaxRequestSize);
Http::RequestParser parser(Const::DefaultMaxRequestSize);
auto feed = [&parser](const char *data) {
parser.feed(data, std::strlen(data));
......@@ -38,6 +38,36 @@ TEST(http_parsing_test, should_parse_http_request_in_two_packets_issue_160) {
ASSERT_EQ(parser.parse(), Http::Private::State::Done);
}
TEST(http_parsing_test, parser_reset) {
Http::RequestParser parser(Const::DefaultMaxRequestSize);
auto feed = [&parser](const char *data) {
parser.feed(data, std::strlen(data));
};
feed("GET /hello?parameter=value HTTP/1.1\r\n");
feed("User-Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, "
"like Gecko) Chrome/41.0.2228.0 Safari/537.36\r\n");
feed("Host: localhost\r\n");
feed("Content-Length: 5\r\n");
feed("\r\n");
feed("HELLO");
ASSERT_EQ(parser.parse(), Http::Private::State::Done);
ASSERT_EQ(parser.request.query().as_str(), "?parameter=value");
ASSERT_EQ(parser.request.resource(), "/hello");
ASSERT_EQ(parser.request.headers().list().size(), 3u);
ASSERT_EQ(parser.request.body(), "HELLO");
parser.reset();
ASSERT_EQ(parser.request.query().as_str(), "");
ASSERT_EQ(parser.request.resource(), "");
ASSERT_EQ(parser.request.headers().list().size(), 0u);
ASSERT_EQ(parser.request.body(), "");
}
TEST(http_parsing_test, succ_response_line_step) {
Http::Response response;
Http::Private::ResponseLineStep step(&response);
......@@ -160,4 +190,4 @@ TEST(http_parsing_test, succ_method_parse_request) {
ASSERT_THROW(step.apply(cursor), Http::HttpError);
}
}
}
\ No newline at end of file
}
......@@ -8,6 +8,7 @@
#include <pistache/endpoint.h>
#include <pistache/http.h>
#include <pistache/peer.h>
#include <pistache/router.h>
#include "httplib.h"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment