Skip to content

Commit

Permalink
fixed the bug: when nginx is reloaded, work_process cann't exit for l…
Browse files Browse the repository at this point in the history 
…ong time
  • Loading branch information
@xiaokai-wang
xiaokai-wang committed Jun 25, 2018
1 parent d050059 commit 75b4a12
Showing 1 changed file with 33 additions and 14 deletions.
47 changes: 33 additions & 14 deletions src/ngx_http_upsync_module.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,15 @@ static ngx_int_t ngx_http_upsync_add_peers(ngx_cycle_t *cycle,
ngx_http_upsync_server_t *upsync_server);
static ngx_int_t ngx_http_upsync_del_peers(ngx_cycle_t *cycle,
ngx_http_upsync_server_t *upsync_server);
static ngx_int_t ngx_http_upsync_replace_peers(ngx_cycle_t *cycle,
ngx_http_upsync_server_t *upsync_server);
static void ngx_http_upsync_update_peer(ngx_http_upstream_rr_peers_t *peers,
ngx_http_upstream_rr_peer_t *peer,
ngx_http_upsync_conf_t *upstream_conf,
ngx_uint_t *updated);
static void ngx_http_upsync_diff_filter(ngx_cycle_t *cycle,
ngx_http_upsync_server_t *upsync_server,
ngx_uint_t *diff);
static ngx_int_t ngx_http_upsync_replace_peers(ngx_cycle_t *cycle,
ngx_http_upsync_server_t *upsync_server);

static void ngx_http_upsync_event_init(ngx_http_upstream_rr_peer_t *peer,
ngx_http_upsync_server_t *upsync_server);
Expand Down Expand Up @@ -923,6 +927,7 @@ ngx_http_upsync_add_peers(ngx_cycle_t *cycle,
return NGX_ERROR;
}


static void
ngx_http_upsync_update_peer(ngx_http_upstream_rr_peers_t *peers,
ngx_http_upstream_rr_peer_t *peer,
Expand Down Expand Up @@ -958,19 +963,21 @@ ngx_http_upsync_update_peer(ngx_http_upstream_rr_peers_t *peers,
return;
}


static void
ngx_http_upsync_diff_filter(ngx_cycle_t *cycle,
ngx_http_upsync_server_t *upsync_server,
ngx_uint_t *diff)
{
ngx_uint_t i, j, len, updated;
ngx_uint_t *flags = NULL;
ngx_array_t flag_array;
ngx_http_upsync_ctx_t *ctx;
ngx_http_upsync_conf_t *upstream_conf, *add_upstream, *del_upstream;
ngx_http_upsync_conf_t *upstream_conf;
ngx_http_upsync_conf_t *add_upstream, *del_upstream;
ngx_http_upstream_rr_peer_t *peer = NULL;
ngx_http_upstream_rr_peers_t *peers = NULL;
ngx_http_upstream_srv_conf_t *uscf;
ngx_uint_t *flags = NULL;
ngx_array_t flag_array;

*diff = 0;
ctx = &upsync_server->ctx;
Expand All @@ -983,14 +990,14 @@ ngx_http_upsync_diff_filter(ngx_cycle_t *cycle,
sizeof(*add_upstream)) != NGX_OK)
{
ngx_log_error(NGX_LOG_ERR, cycle->log, 0,
"upsync_add_check: alloc error");
"upsync_diff_filter_add: alloc error");
return;
}

if (ngx_array_init(&ctx->del_upstream, ctx->pool, 16,
sizeof(*del_upstream)) != NGX_OK) {
ngx_log_error(NGX_LOG_ERR, cycle->log, 0,
"upsync_del_check: alloc error");
"upsync_diff_filter_del: alloc error");
return;
}

Expand All @@ -1000,12 +1007,11 @@ ngx_http_upsync_diff_filter(ngx_cycle_t *cycle,
}

peers = (ngx_http_upstream_rr_peers_t *)uscf->peer.data;

if (peers->number != 0) {
if (ngx_array_init(&flag_array, ctx->pool, peers->number,
sizeof(*flags)) != NGX_OK) {
ngx_log_error(NGX_LOG_ERR, cycle->log, 0,
"upsync_del_flags: alloc error");
"upsync_diff_filter: alloc error");
return;
}

Expand All @@ -1021,17 +1027,17 @@ ngx_http_upsync_diff_filter(ngx_cycle_t *cycle,
if (*(flags + j) == 1) {
continue;
}

if (ngx_memn2cmp(peer->name.data, upstream_conf->sockaddr,
peer->name.len,
ngx_strlen(upstream_conf->sockaddr)) == 0) {
// update peer
ngx_http_upsync_update_peer(peers, peer, upstream_conf, &updated);
*diff |= updated;

// set flag, not to be deleted
*(flags + j) = 1;

break;
}
}
Expand All @@ -1048,7 +1054,7 @@ ngx_http_upsync_diff_filter(ngx_cycle_t *cycle,
if (*(flags + j) == 1) {
continue;
}

del_upstream = ngx_array_push(&ctx->del_upstream);
ngx_memzero(del_upstream, sizeof(*del_upstream));
ngx_memcpy(&del_upstream->sockaddr, peer->name.data, peer->name.len);
Expand Down Expand Up @@ -3485,7 +3491,7 @@ ngx_http_upsync_timeout_handler(ngx_event_t *event)
upsync_server = event->data;

ngx_log_error(NGX_LOG_ERR, event->log, 0,
"upsync_timeout: timed out reading upsync_server: %V ",
"[WARN] upsync_timeout: timed out reading upsync_server: %V ",
upsync_server->pc.name);

ngx_http_upsync_clean_event(upsync_server);
Expand Down Expand Up @@ -3566,7 +3572,9 @@ static void
ngx_http_upsync_clear_all_events(ngx_cycle_t *cycle)
{
ngx_uint_t i;
ngx_queue_t *head, *next;
ngx_connection_t *c;
ngx_delay_event_t *queue_event;
ngx_upsync_conf_t *upsync_type_conf;
ngx_http_upsync_server_t *upsync_server;

Expand Down Expand Up @@ -3598,6 +3606,17 @@ ngx_http_upsync_clear_all_events(ngx_cycle_t *cycle)
}
ngx_del_timer(&upsync_server[i].upsync_timeout_ev);
}

head = &upsync_server[i].delete_ev;
for (next = ngx_queue_head(head);
next != ngx_queue_sentinel(head);
next = ngx_queue_next(next)) {

queue_event = ngx_queue_data(next, ngx_delay_event_t, delay_delete_ev);
if (queue_event->delay_delete_ev.timer_set) {
ngx_del_timer(&queue_event->delay_delete_ev);
}
}
}

if (upsync_type_conf->upsync_type == NGX_HTTP_UPSYNC_CONSUL
Expand Down

3 comments on commit 75b4a12

@gfrankliu
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit seems to cause segfault sometimes, as discussed in #258 , some other times it doesn't cause segfault but the worker process will stay in shutting down mode for long time (~30min), the symptom that this commit is supposed to fix :)

@xiaokai-wang
Copy link
Member Author

@xiaokai-wang xiaokai-wang commented on 75b4a12 Sep 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit is only changed to clear the timer which is used to delete the outdated upstream server. And then the function 'ngx_http_upsync_clear_all_events' only be executed when the nginx restarted/stoped and only be executed one time.
As you said @gfrankliu , It sometimes cored and sometimes it's not worked which will be still shutting down long time. Both are not normal.

ngx_module_t  ngx_http_upsync_module = {
    NGX_MODULE_V1,
    &ngx_http_upsync_module_ctx,                /* module context */
    ngx_http_upsync_commands,                   /* module directives */
    NGX_HTTP_MODULE,                            /* module type */
    NULL,                                       /* init master */
    ngx_http_upsync_init_module,                /* init module */
    ngx_http_upsync_init_process,               /* init process */
    NULL,                                       /* init thread */
    NULL,                                       /* exit thread */
    ngx_http_upsync_clear_all_events,           /* exit process */
    NULL,                                       /* exit master */
    NGX_MODULE_V1_PADDING
};

As code shows above, 'ngx_http_upsync_clear_all_events' will be executed when the process exited. So it's not be still shutting down mode for long time theoretically.

I hope you can add some logs to see if the code of the commit is executed normally.
Thanks @gfrankliu

@gfrankliu
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Details are posted in #258

Also, this is easily reproducible. You can issue reload a few times and see either one of the two behaviors (crash or hang). Are you not seeing it in your test?

Please sign in to comment.