Fix releasing not dead sock #2239
Conversation
74be1ef
to
675c29b
Compare
fw/sock_srv.c
Outdated
| @@ -277,6 +277,9 @@ tfw_sock_srv_connect_try(TfwSrvConn *srv_conn) | |||
| if (r != -ESHUTDOWN) | |||
| T_ERR("Unable to initiate a connect to server: %d\n", | |||
| r); | |||
| bh_lock_sock(sk); | |||
| ss_do_close(sk, 0); | |||
| bh_unlock_sock(sk); | |||
| sock_put(sk); | |||
| SS_CALL(connection_drop, sk); | |||
There was a problem hiding this comment.
Why not ss_close()? It calls also sock_put() and connection_drop. It's error prone and breaks the abstraction layers w/o a clear reason
There was a problem hiding this comment.
Because here socket state is SS_CLOSE, but we should not call ss_do_close for socket in this state in all other places except here. We can add special flag for such socket, but I think that this solution is better
There was a problem hiding this comment.
I don't understand
Because here socket state is SS_CLOSE, but we should not call ss_do_close for socket in this state in all other places except here.
Yes, we discuss this particular place in tfw_sock_srv_connect_try().
My objection that here are 5 lines of code
bh_lock_sock(sk);
ss_do_close(sk, 0);
bh_unlock_sock(sk);
sock_put(sk);
SS_CALL(connection_drop, sk);dealing with low-level socket API - this is subject for sock.c, no sock_src.c, which should care about connection management and work with connection abstractions.
What would be wrong if we replace these 5 lines with ss_close()?
There was a problem hiding this comment.
look at the ss_tx_action case SS_CLOSE. we check that:
if (!((1 << sk->sk_state)
& (TCPF_ESTABLISHED | TCPF_SYN_SENT
| TCPF_CLOSE_WAIT)))
and if this if is true don't call ss_do_close. It is necessary to prevent multiple socket closing. But socket in tfw_sock_srv_connect_try has CLOSE state and this if is true. So if we call ss_close from tfw_sock_srv_connect_try instead of this five lines of code we never drop connection and close socket! We can pass special flag in ss_tx_action but I prefer this way. If you don't think so ok I will change it
82921fa
to
a33a0ca
Compare
0beab74
to
2d33a7b
Compare
When `ss_connect` fails we put socket refcounter and release sock (when it becames equal to zero), but socket was not closed. That leads to error message in dmesg and some socket resources could be not freed. This patch fix this behaviour, now we close socket before free it.
We allocate resources for cache if condition `cache_cfg.cache || g_vhost->cache_purge` is true, so we should dealloate it also if this condition is true.
- Tempesta FW fails to start if cache directive is empty or equal to zero, but purge directive is set. - Tempesta FW fails to start if purge directive is set but purge acl is not set and vice versa. - Implement new function `ss_close_not_connected_socket` to close sockets in TCP_CLOSE state (which are not DEAD), to handle a situation when we create socket using `ss_inet_create` in TCP_CLOSE state and need to close such socket.
2d33a7b
to
02380f5
Compare
When
ss_connectfails we put socket refcounterand release sock (when it becames equal to zero),
but socket was not closed. That leads to error
message in dmesg and some socket resources could
be not freed. This patch fix this behaviour, now
we close socket before free it.