Fix large packets fails; Change memory leak case

Author: dmytro-rezchykov-tr

ngx_http_websocket_stat_frame_counter.c

@@ -112,7 +112,7 @@ frame_counter_process_message(u_char **buffer, ssize_t *size,
             } else {
                 frame_counter->bytes_consumed += *size;
                 if (frame_counter->bytes_consumed >
-                    frame_counter->total_payload_size) {
+                    frame_counter->current_payload_size) {
                     ngx_log_error(NGX_LOG_ERR, ngx_cycle->log, 0, "WTF?");
                     frame_counter->stage = HEADER;
                 }

ngx_http_websocket_stat_frame_counter.h

@@ -15,10 +15,7 @@ typedef enum { CONTINUATION, TEXT, BINARY, CLOSE = 8, PING, PONG } frame_type;
 
 // Structure representing frame statistic and parsing stage
 typedef struct {
-    // statistic fields
-    ngx_int_t frames;
     ngx_int_t total_payload_size;
-    ngx_int_t total_size;
 
     // private fields representing current parcing stage
     ngx_int_t bytes_consumed;

ngx_http_websocket_stat_module.c

@@ -7,14 +7,24 @@
 
 typedef struct {
     time_t ws_conn_start_time;
+    ngx_frame_counter_t frame_counter;
+
 } ngx_http_websocket_stat_ctx;
 
+typedef struct {
+    ngx_int_t frames;
+    ngx_int_t total_payload_size;
+    ngx_int_t total_size;
+} ngx_http_websocket_stat_statistic_t;
+
+ngx_http_websocket_stat_statistic_t frames_in;
+ngx_http_websocket_stat_statistic_t frames_out;
+
 ngx_frame_counter_t frame_counter_in;
 ngx_frame_counter_t frame_counter_out;
 
 ngx_http_websocket_stat_ctx *stat_counter;
 typedef struct {
-    ngx_frame_counter_t *counter;
     int from_client;
     ngx_http_websocket_stat_ctx *ws_ctx;
 
@@ -137,9 +147,9 @@ ngx_http_websocket_stat_handler(ngx_http_request_t *r)
     out.buf = b;
     out.next = NULL;
     sprintf((char *)msg, (char *)responce_template, ngx_websocket_stat_active,
-            frame_counter_in.frames, frame_counter_in.total_payload_size,
-            frame_counter_in.total_size, frame_counter_out.frames,
-            frame_counter_out.total_payload_size, frame_counter_out.total_size);
+            frames_in.frames, frames_in.total_payload_size,
+            frames_in.total_size, frames_out.frames,
+            frames_out.total_payload_size, frames_out.total_size);
 
     b->pos = msg; /* first position in memory of the data */
     b->last =
@@ -210,20 +220,20 @@ my_send(ngx_connection_t *c, u_char *buf, size_t size)
     ngx_http_websocket_stat_ctx *ctx;
     ssize_t sz = size;
     u_char *buffer = buf;
-    ngx_frame_counter_t *frame_counter = &frame_counter_out;
+    ngx_http_websocket_stat_statistic_t *frame_counter = &frames_out;
     frame_counter->total_size += sz;
     ngx_http_request_t *r = c->data;
 
     ctx = ngx_http_get_module_ctx(r, ngx_http_websocket_stat_module);
     template_ctx_s template_ctx;
-    template_ctx.counter = frame_counter;
     template_ctx.from_client = 0;
     template_ctx.ws_ctx = ctx;
     while (sz > 0) {
-        if (frame_counter_process_message(&buffer, &sz, frame_counter)) {
+        if (frame_counter_process_message(&buffer, &sz,
+                                          &(ctx->frame_counter))) {
             frame_counter->frames++;
             frame_counter->total_payload_size +=
-                frame_counter->current_payload_size;
+                ctx->frame_counter.current_payload_size;
             char *log_line = apply_template(log_template, r, &template_ctx);
             websocket_log(log_line);
             free(log_line);
@@ -241,19 +251,18 @@ my_recv(ngx_connection_t *c, u_char *buf, size_t size)
 
     ngx_http_websocket_stat_ctx *ctx;
     ssize_t sz = n;
-    ngx_frame_counter_t *frame_counter = &frame_counter_in;
+    ngx_http_websocket_stat_statistic_t *frame_counter = &frames_in;
     ngx_http_request_t *r = c->data;
     ctx = ngx_http_get_module_ctx(r, ngx_http_websocket_stat_module);
     frame_counter->total_size += n;
     template_ctx_s template_ctx;
-    template_ctx.counter = frame_counter;
     template_ctx.from_client = 1;
     template_ctx.ws_ctx = ctx;
     while (sz > 0) {
-        if (frame_counter_process_message(&buf, &sz, frame_counter)) {
+        if (frame_counter_process_message(&buf, &sz, &ctx->frame_counter)) {
             frame_counter->frames++;
             frame_counter->total_payload_size +=
-                frame_counter->current_payload_size;
+                ctx->frame_counter.current_payload_size;
             if (ws_log) {
                 char *log_line = apply_template(log_template, r, &template_ctx);
                 websocket_log(log_line);
@@ -274,7 +283,6 @@ ngx_http_websocket_stat_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
     ngx_http_websocket_stat_ctx *ctx;
     ctx = ngx_http_get_module_ctx(r, ngx_http_websocket_stat_module);
     template_ctx_s template_ctx;
-    template_ctx.counter = NULL;
     template_ctx.ws_ctx = ctx;
 
     if (r->upstream->upgrade) {
@@ -317,7 +325,7 @@ const char *
 ws_packet_type(ngx_http_request_t *r, void *data)
 {
     template_ctx_s *ctx = data;
-    ngx_frame_counter_t *frame_cntr = ctx->counter;
+    ngx_frame_counter_t *frame_cntr = &(ctx->ws_ctx->frame_counter);
     if (!ctx || !frame_cntr)
         return UNKNOWN_VAR;
     sprintf(buff, "%d", frame_cntr->current_frame_type);
@@ -328,7 +336,7 @@ const char *
 ws_packet_size(ngx_http_request_t *r, void *data)
 {
     template_ctx_s *ctx = data;
-    ngx_frame_counter_t *frame_cntr = ctx->counter;
+    ngx_frame_counter_t *frame_cntr = &ctx->ws_ctx->frame_counter;
     if (!ctx || !frame_cntr)
         return UNKNOWN_VAR;
     sprintf(buff, "%lu", frame_cntr->current_payload_size);

test/unit_test.py

@@ -37,7 +37,6 @@ def regularCheck(self, sent_frames, sent_payload,
         self.assertEqual(logged_payload, reported_payload)
         self.assertEqual(connections, 0)
 
-    @unittest.skip("")
     def testSimple(self):
         self_run_cmd = local["python3"]['test/ws_test.py'] \
                        [
@@ -52,7 +51,6 @@ def testSimple(self):
                        ]
         self.regularCheck(*[int(x) for x in self_run_cmd().split()])
 
-    @unittest.skip("")
     def test500Cons(self):
         self_run_cmd = local["python3"]['test/ws_test.py'] \
                        [
@@ -67,7 +65,6 @@ def test500Cons(self):
                        ]
         self.regularCheck(*[int(x) for x in self_run_cmd().split()])
 
-    @unittest.skip("")
     def testLongRun500Cons(self):
         self_run_cmd = local["python3"]['test/ws_test.py'] \
                        [
@@ -82,7 +79,6 @@ def testLongRun500Cons(self):
                        ]
         self.regularCheck(*[int(x) for x in self_run_cmd().split()])
 
-    @unittest.skip("")
     def testLargePackets(self):
         self_run_cmd = local["python3"]['test/ws_test.py'] \
                        [
@@ -91,7 +87,7 @@ def testLargePackets(self):
                        "--fps", 3,
                        "--seconds", 30,
                        "--connections", 5,
-                       "--packet", 3000,
+                       "--packet", 5000,
                        "--instances", 100,
                        "--robot_friendly"
                        ]
@@ -113,15 +109,21 @@ def testMemoryLeak(self):
                        "--robot_friendly",
                        "--keepNginx"
                        ]
+        print("First run")
         self_run_cmd()
         self.assertEqual(pid, getNginxPID())
-        memAfter = getTotalMem(pid)
+        mem1stRun = getTotalMem(pid)
+        print("Second run")
         self_run_cmd()
         self.assertEqual(pid, getNginxPID())
         mem2ndRun = getTotalMem(pid)
-        print("Mem before: {}, Mem after: {}, mem2ndrun: {}".format(memBefore, memAfter, mem2ndRun))
-        self.assertTrue(memAfter - memBefore <= 24)
-        self.assertEqual(memAfter, mem2ndRun)
+        print("Third run")
+        self_run_cmd()
+        self.assertEqual(pid, getNginxPID())
+        mem3rdRun = getTotalMem(pid)
+        self.assertTrue(mem1stRun - memBefore <= 24)
+        self.assertEqual(mem1stRun, mem2ndRun)
+        self.assertEqual(mem2ndRun, mem3rdRun)
 
 if __name__ == "__main__":
     f = local["python3"]["test/test_server.py"] & BG