From dbb70423608c8f7e09aa1368f69f19b9c9d57be0 Mon Sep 17 00:00:00 2001 From: Georg Hopp Date: Sun, 19 Feb 2012 18:15:55 +0100 Subject: [PATCH] fixed the non keep-alive performance issue as well as i lower memory usage by using a single read and write circular buffer for every connection. @TODO: i noticed a server hang while getting large data (my image) with non keep-alive connections. Additionally an incomplete keep-alive request might stop the server now as the lock on the read buffer will not be released. --- include/cbuf.h | 11 +++++++++ include/http/request/parser.h | 20 ++++++----------- include/http/response/writer.h | 9 +++++++- include/http/worker.h | 15 +++++++++++++ src/Makefile.am | 2 +- src/cbuf/get_data.c | 2 +- src/cbuf/is_locked.c | 9 ++++++++ src/cbuf/lock.c | 9 ++++++++ src/cbuf/release.c | 9 ++++++++ src/cbuf/set_data.c | 2 +- src/http/request/parser.c | 10 ++++----- src/http/request/parser/parse.c | 22 ++++++++++++------ src/http/response/writer.c | 21 +++++------------- src/http/response/writer/write.c | 17 +++++++++++++- src/http/worker.c | 38 +++++++++++++++++++++++++++----- src/server/handle_accept.c | 18 ++++++++++----- src/testserver.c | 6 ++--- 17 files changed, 158 insertions(+), 62 deletions(-) create mode 100644 src/cbuf/is_locked.c create mode 100644 src/cbuf/lock.c create mode 100644 src/cbuf/release.c diff --git a/include/cbuf.h b/include/cbuf.h index 4a08154..0c2a7e4 100644 --- a/include/cbuf.h +++ b/include/cbuf.h @@ -18,11 +18,19 @@ #define ECBUFOVFL 100 +#ifndef TRUE +#define TRUE ((void *)1) +#endif + +#ifndef FALSE +#define FALSE ((void *)0) +#endif CLASS(Cbuf) { char * shm_name; // shared memory identifier char * data; + void * lock; size_t bsize; size_t bused; @@ -47,6 +55,9 @@ void cbufIncWrite(Cbuf this, size_t n); size_t cbufGetFree(Cbuf this); char cbufIsEmpty(Cbuf this); void cbufSkipNonAlpha(Cbuf this); +void * cbufIsLocked(Cbuf this); +void cbufLock(Cbuf this); +void cbufRelease(Cbuf this); #endif // __RINGBUFFER_H__ diff --git a/include/http/request/parser.h b/include/http/request/parser.h index 412df5b..8fa51ff 100644 --- a/include/http/request/parser.h +++ b/include/http/request/parser.h @@ -6,20 +6,13 @@ #include "http/message/queue.h" #include "cbuf.h" -#define HTTP_REQUEST_PARSER_BUFFER_MAX 8192 +#ifndef TRUE +#define TRUE ((void *)1) +#endif - -/** - * limits to stop invalid requests from killing - * the server. - * If any of these limits is reached the server - * will send an error message and kill the connection - * immediate. - * - * The given limits include any trailing \r\n - */ -#define HTTP_REQUEST_LINE_MAX 8192 -#define HTTP_REQUEST_HEADER_LINE_MAX 2048 +#ifndef FALSE +#define FALSE ((void *)0) +#endif typedef enum e_HttpRequestState { @@ -33,6 +26,7 @@ typedef enum e_HttpRequestState { CLASS(HttpRequestParser) { Cbuf buffer; + void * ourLock; HttpMessageQueue request_queue; HttpRequest cur_request; diff --git a/include/http/response/writer.h b/include/http/response/writer.h index c3e9b81..995817c 100644 --- a/include/http/response/writer.h +++ b/include/http/response/writer.h @@ -8,7 +8,13 @@ #include "http/message/queue.h" #include "cbuf.h" -#define RESPONSE_WRITER_MAX_BUF 131072 +#ifndef TRUE +#define TRUE ((void *)1) +#endif + +#ifndef FALSE +#define FALSE ((void *)0) +#endif typedef enum e_HttpResponseState { @@ -19,6 +25,7 @@ typedef enum e_HttpResponseState { CLASS(HttpResponseWriter) { Cbuf buffer; + void * ourLock; HttpMessageQueue response_queue; HttpResponse cur_response; diff --git a/include/http/worker.h b/include/http/worker.h index 9040b0c..86199b3 100644 --- a/include/http/worker.h +++ b/include/http/worker.h @@ -6,10 +6,25 @@ #include "class.h" #include "http/request/parser.h" #include "http/response/writer.h" +#include "cbuf.h" + +#define RESPONSE_WRITER_MAX_BUF 32768 +#define REQUEST_PARSER_BUFFER_MAX 8192 + +#ifndef TRUE +#define TRUE ((void *)1) +#endif + +#ifndef FALSE +#define FALSE ((void *)0) +#endif CLASS(HttpWorker) { char * id; + Cbuf pbuf; + Cbuf wbuf; + HttpRequestParser parser; HttpResponseWriter writer; }; diff --git a/src/Makefile.am b/src/Makefile.am index 041880c..2d2256a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -13,7 +13,7 @@ CB = cbuf.c cbuf/read.c cbuf/write.c \ cbuf/get_line.c cbuf/set_data.c cbuf/get_data.c \ cbuf/addr_index.c cbuf/get_free.c cbuf/get_read.c cbuf/get_write.c \ cbuf/inc_read.c cbuf/inc_write.c cbuf/is_empty.c cbuf/memchr.c \ - cbuf/skip_non_alpha.c + cbuf/skip_non_alpha.c cbuf/is_locked.c cbuf/lock.c cbuf/release.c MSG = http/message.c http/message/queue.c http/message/has_keep_alive.c \ http/message/header_size_get.c http/message/header_to_string.c REQ = http/request.c diff --git a/src/cbuf/get_data.c b/src/cbuf/get_data.c index 0ebdb9f..9a3c0d5 100644 --- a/src/cbuf/get_data.c +++ b/src/cbuf/get_data.c @@ -9,7 +9,7 @@ cbufGetData(Cbuf this, size_t n) char * ret = cbufGetRead(this); if (n > this->bused) { - return -1; + return (char *)-1; } cbufIncRead(this, n); diff --git a/src/cbuf/is_locked.c b/src/cbuf/is_locked.c new file mode 100644 index 0000000..4d2f441 --- /dev/null +++ b/src/cbuf/is_locked.c @@ -0,0 +1,9 @@ +#include "cbuf.h" + +void * +cbufIsLocked(Cbuf this) +{ + return this->lock; +} + +// vim: set ts=4 sw=4: diff --git a/src/cbuf/lock.c b/src/cbuf/lock.c new file mode 100644 index 0000000..e845dca --- /dev/null +++ b/src/cbuf/lock.c @@ -0,0 +1,9 @@ +#include "cbuf.h" + +void +cbufLock(Cbuf this) +{ + this->lock = TRUE; +} + +// vim: set ts=4 sw=4: diff --git a/src/cbuf/release.c b/src/cbuf/release.c new file mode 100644 index 0000000..147fddb --- /dev/null +++ b/src/cbuf/release.c @@ -0,0 +1,9 @@ +#include "cbuf.h" + +void +cbufRelease(Cbuf this) +{ + this->lock = FALSE; +} + +// vim: set ts=4 sw=4: diff --git a/src/cbuf/set_data.c b/src/cbuf/set_data.c index 1748c66..4b3f4ee 100644 --- a/src/cbuf/set_data.c +++ b/src/cbuf/set_data.c @@ -11,7 +11,7 @@ cbufSetData(Cbuf this, const void * src, size_t n) if (n > cbufGetFree(this)) { errno = ECBUFOVFL; - return -1; + return (char *)-1; } addr = memcpy(cbufGetWrite(this), src, n); diff --git a/src/http/request/parser.c b/src/http/request/parser.c index bf0d2f0..3a067e2 100644 --- a/src/http/request/parser.c +++ b/src/http/request/parser.c @@ -18,12 +18,8 @@ void ctor(void * _this, va_list * params) { HttpRequestParser this = _this; - char * id = va_arg(*params, char*); - char cbuf_id[100]; - sprintf(cbuf_id, "%s_%s", "parser", id); - - this->buffer = new(Cbuf, cbuf_id, HTTP_REQUEST_PARSER_BUFFER_MAX); + this->buffer = va_arg(* params, Cbuf); this->request_queue = new(HttpMessageQueue); } @@ -34,7 +30,9 @@ dtor(void * _this) HttpRequestParser this = _this; delete(&(this->request_queue)); - delete(&(this->buffer)); + + if (TRUE == this->ourLock) + cbufRelease(this->buffer); if (NULL != this->cur_request) delete(&(this->cur_request)); diff --git a/src/http/request/parser/parse.c b/src/http/request/parser/parse.c index d04990f..d24c510 100644 --- a/src/http/request/parser/parse.c +++ b/src/http/request/parser/parse.c @@ -16,6 +16,15 @@ httpRequestParserParse(HttpRequestParser this, int fd) ssize_t read; char * line; + if (cbufIsLocked(this->buffer)) { + if (FALSE == this->ourLock) + return 0; + } + else { + cbufLock(this->buffer); + this->ourLock = TRUE; + } + if(0 > (read = cbufRead(this->buffer, fd))) { return read; } @@ -28,6 +37,12 @@ httpRequestParserParse(HttpRequestParser this, int fd) this->cur_request = new(HttpRequest); this->state = HTTP_REQUEST_START; } + else { + cbufRelease(this->buffer); + this->ourLock = FALSE; + cont = 0; + } + break; case HTTP_REQUEST_START: @@ -98,13 +113,6 @@ httpRequestParserParse(HttpRequestParser this, int fd) this->cur_request = NULL; - /** - * dont continue loop if input buffer is empty - */ - if (cbufIsEmpty(this->buffer)) { - cont = 0; - } - /** * prepare for next request */ diff --git a/src/http/response/writer.c b/src/http/response/writer.c index 564d9f9..e2d9587 100644 --- a/src/http/response/writer.c +++ b/src/http/response/writer.c @@ -13,12 +13,8 @@ void ctor(void * _this, va_list * params) { HttpResponseWriter this = _this; - char * id = va_arg(*params, char*); - char cbuf_id[100]; - sprintf(cbuf_id, "%s_%s", "writer", id); - - this->buffer = new(Cbuf, cbuf_id, RESPONSE_WRITER_MAX_BUF); + this->buffer = va_arg(*params, Cbuf); this->response_queue = new(HttpMessageQueue); } @@ -29,22 +25,15 @@ dtor(void * _this) HttpResponseWriter this = _this; delete(&(this->response_queue)); - delete(&(this->buffer)); + + if (TRUE == this->ourLock) + cbufRelease(this->buffer); if (NULL != this->cur_response) delete(&(this->cur_response)); } -static -void -_clone(void * _this, void * _base) -{ - HttpResponseWriter this = _this; - - this->response_queue = new(HttpMessageQueue); -} - -INIT_IFACE(Class, ctor, dtor, _clone); +INIT_IFACE(Class, ctor, dtor, NULL); INIT_IFACE(StreamWriter, (fptr_streamWriterWrite)httpResponseWriterWrite); CREATE_CLASS(HttpResponseWriter, NULL, IFACE(Class), IFACE(StreamWriter)); diff --git a/src/http/response/writer/write.c b/src/http/response/writer/write.c index 27c9972..3fcdbc6 100644 --- a/src/http/response/writer/write.c +++ b/src/http/response/writer/write.c @@ -25,6 +25,15 @@ httpResponseWriterWrite(HttpResponseWriter this, int fd) HttpMessage message = (HttpMessage)this->cur_response; int cont = 1; + if (cbufIsLocked(this->buffer)) { + if (FALSE == this->ourLock) + return 0; + } + else { + cbufLock(this->buffer); + this->ourLock = TRUE; + } + while (cont) { switch (this->state) { case HTTP_RESPONSE_GET: @@ -42,7 +51,9 @@ httpResponseWriterWrite(HttpResponseWriter this, int fd) this->state = HTTP_RESPONSE_WRITE; } else { - cont = 0; + cbufRelease(this->buffer); + this->ourLock = FALSE; + cont = 0; } break; @@ -113,10 +124,14 @@ httpResponseWriterWrite(HttpResponseWriter this, int fd) * return to the caller with a 0 indicating that the * underlying connection should be closed. */ + cbufRelease(this->buffer); + this->ourLock = FALSE; delete(&this->cur_response); return -1; } + cbufRelease(this->buffer); + this->ourLock = FALSE; delete(&this->cur_response); break; diff --git a/src/http/worker.c b/src/http/worker.c index 5a3f0ce..04bb426 100644 --- a/src/http/worker.c +++ b/src/http/worker.c @@ -2,6 +2,7 @@ #include #include #include +#include #include "class.h" #include "http/worker.h" @@ -19,12 +20,19 @@ ctor(void * _this, va_list * params) { HttpWorker this = _this; char * id = va_arg(*params, char *); + char cbuf_id[100]; this->id = malloc(strlen(id) + 1); strcpy(this->id, id); - this->parser = new(HttpRequestParser, this->id); - this->writer = new(HttpResponseWriter, this->id); + sprintf(cbuf_id, "%s_%s", "parser", id); + this->pbuf = new(Cbuf, cbuf_id, REQUEST_PARSER_BUFFER_MAX); + + sprintf(cbuf_id, "%s_%s", "writer", id); + this->wbuf = new(Cbuf, cbuf_id, RESPONSE_WRITER_MAX_BUF); + + this->parser = new(HttpRequestParser, this->pbuf); + this->writer = new(HttpResponseWriter, this->wbuf); } static @@ -33,13 +41,31 @@ dtor(void * _this) { HttpWorker this = _this; - free(this->id); + if (NULL != this->id) free(this->id); + + delete(&(this->parser)); + delete(&(this->writer)); + + if (NULL != this->pbuf) delete(&(this->pbuf)); + if (NULL != this->wbuf) delete(&(this->wbuf)); +} + +static +void +_clone(void * _this, void * _base) +{ + HttpWorker this = _this; + HttpWorker base = _base; + + this->id = NULL; + this->pbuf = NULL; + this->wbuf = NULL; - delete(&this->parser); - delete(&this->writer); + this->parser = new(HttpRequestParser, base->pbuf); + this->writer = new(HttpResponseWriter, base->wbuf); } -INIT_IFACE(Class, ctor, dtor, NULL); +INIT_IFACE(Class, ctor, dtor, _clone); INIT_IFACE(StreamReader, (fptr_streamReaderRead)httpWorkerProcess); INIT_IFACE(StreamWriter, (fptr_streamWriterWrite)httpWorkerWrite); CREATE_CLASS( diff --git a/src/server/handle_accept.c b/src/server/handle_accept.c index ebd3063..53a2b44 100644 --- a/src/server/handle_accept.c +++ b/src/server/handle_accept.c @@ -1,5 +1,6 @@ #include #include +#include #include "http/worker.h" @@ -16,15 +17,20 @@ serverHandleAccept(Server this) acc = socketAccept(this->sock, &remoteAddr); if (-1 != acc->handle) { - char id[21]; - - sprintf(id, "my_%s_%05d", remoteAddr, acc->handle); - //* save the socket handle (this->conns)[acc->handle].sock = acc; - //* clone reader - (this->conns)[acc->handle].worker = new(HttpWorker, id); + //* clone worker + (this->conns)[acc->handle].worker = clone(this->worker); + + /** + * @TODO: + * set worker id---this would better be accomplished + * as a worker method. + * The same is true for server information stuff only + * that this should become an interface. + * At a second thought both has to be an interface. + */ (this->fds)[this->nfds].fd = acc->handle; (this->fds)[this->nfds].events = POLLIN; diff --git a/src/testserver.c b/src/testserver.c index 7289993..634ea7e 100644 --- a/src/testserver.c +++ b/src/testserver.c @@ -19,8 +19,8 @@ int main() { Logger logger = new(LoggerSyslog, LOGGER_ERR); - //HttpWorker worker = new(HttpWorker); - Server server = new(Server, logger, NULL /*worker*/, 11212, SOMAXCONN); + HttpWorker worker = new(HttpWorker, "my"); + Server server = new(Server, logger, worker, 11212, SOMAXCONN); struct rlimit limit = {RLIM_INFINITY, RLIM_INFINITY}; setrlimit(RLIMIT_CPU, &limit); @@ -30,7 +30,7 @@ main() serverRun(server); delete(&server); -// delete(&worker); + delete(&worker); delete(&logger); return 0;