-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed memory leak of ssl session reuse in dyups and session sticky module #1708
Conversation
try patch: https://patch-diff.githubusercontent.com/raw/alibaba/tengine/pull/1708.patch firstly |
|
It reuses ssl session reuse of round robin module now, and removes duplicated codes in session sticky module.
@@ -246,16 +234,8 @@ ngx_http_upstream_session_sticky_init_peer(ngx_http_request_t *r, | |||
sspd->sscf = sscf; | |||
sspd->get_rr_peer = ngx_http_upstream_get_round_robin_peer; | |||
|
|||
r->upstream->peer.data = sspd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We make sure that (sspd == &sspd->rr). See:
typedef struct {
/* the round robin data must be first */
ngx_http_upstream_rr_peer_data_t rrp; <- &sspd->rrp
...
ngx_http_upstream_ss_peer_data_t; <- sspd
(refer to nginx core module: ngx_http_upstream_ip_hash_module.c)
ngx_http_upstream_session_sticky_set_peer_session; | ||
r->upstream->peer.save_session = | ||
ngx_http_upstream_session_sticky_save_peer_session; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont need to re-write set_session() and save_session() function when u->peer.data is pointer of ngx_http_upstream_rr_peer_data_t, see ngx_http_upstream_ip_hash_module.c.
Only when u->peer.data is changed to module private data, it needs to re-write these two callbacks. For example, see ngx_http_upstream_keepalive_module.c or dyups module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个module里有不少code style问题,本次pr要不一起调整了?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
has been merged. May next time |
try to fix #1016
todo: added test case for session reuse into dyups.ttodo: added test case for session reuse into session_sticky.t