Skip to content

Commit

Permalink
Fix resuming partial msg reading on TCP
Browse files Browse the repository at this point in the history
When conn rebalancing or parallel processing is enabled for TCP, a read operation may start (partial reading) in one process and be completed in a different one. So the con_req (buffer for partial reading) must be in shm mem and also it must survive a conn release (passing the conn back to MAIN).
Many thanks to @liviuchircu for reporting and helping with the troubleshooting
This issue was found during the TCP performance tests done for the 3.4 release

(cherry picked from commit e0fe732)
  • Loading branch information
bogdan-iancu committed Jun 30, 2023
1 parent 4b902bc commit 5510620
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 14 deletions.
3 changes: 3 additions & 0 deletions net/net_tcp.c
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,9 @@ static void _tcpconn_rm(struct tcp_connection* c, int no_event)
c->async = NULL;
}

if (c->con_req)
shm_free(c->con_req);

if (protos[c->type].net.conn_clean)
protos[c->type].net.conn_clean(c);

Expand Down
8 changes: 0 additions & 8 deletions net/net_tcp_proc.c
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,6 @@ static void tcpconn_release(struct tcp_connection* c, long state, int writer,
c, state, c->fd, c->id);
LM_DBG(" extra_data %p\n", c->extra_data);

/* if we are in a writer context, do not touch the buffer contain read packets per connection
might be in a completely different process
even if in our process we shouldn't touch it, since it might currently be in use, when we've read multiple SIP messages in one try*/
if (!writer && c->con_req) {
pkg_free(c->con_req);
c->con_req = NULL;
}

/* release req & signal the parent */
if (!writer)
c->proc_id = -1;
Expand Down
6 changes: 3 additions & 3 deletions net/proto_tcp/proto_tcp.c
Original file line number Diff line number Diff line change
Expand Up @@ -676,9 +676,9 @@ static int tcp_read_req(struct tcp_connection* con, int* bytes_read)

if (con->con_req) {
req=con->con_req;
LM_DBG("Using the per connection buff \n");
LM_DBG("Using the per connection buff for conn %p\n",con);
} else {
LM_DBG("Using the global ( per process ) buff \n");
LM_DBG("Using the global ( per process ) buff for conn %p\n",con);
init_tcp_req(&tcp_current_req, 0);
req=&tcp_current_req;
}
Expand Down Expand Up @@ -741,7 +741,7 @@ static int tcp_read_req(struct tcp_connection* con, int* bytes_read)
goto error;
}

LM_DBG("tcp_read_req end\n");
LM_DBG("tcp_read_req end for conn %p, req is %p\n",con,con->con_req);
done:
if (bytes_read) *bytes_read=total_bytes;
/* connection will be released */
Expand Down
6 changes: 3 additions & 3 deletions net/proto_tcp/tcp_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ static inline int tcp_handle_req(struct tcp_req *req,
if (req != &_tcp_common_current_req) {
/* if we no longer need this tcp_req
* we can free it now */
pkg_free(req);
shm_free(req);
con->con_req = NULL;
}
} else {
Expand All @@ -469,8 +469,8 @@ static inline int tcp_handle_req(struct tcp_req *req,
if (req == &_tcp_common_current_req) {
/* let's duplicate this - most likely another conn will come in */

LM_DBG("We didn't manage to read a full request\n");
con->con_req = pkg_malloc(sizeof(struct tcp_req));
LM_ERR("We didn't manage to read a full request on con %p\n",con);
con->con_req = shm_malloc(sizeof(struct tcp_req));
if (con->con_req == NULL) {
LM_ERR("No more mem for dynamic con request buffer\n");
goto error;
Expand Down

0 comments on commit 5510620

Please sign in to comment.