From 7d5ae77d840afda65020237f87a4535f09f0b462 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Thu, 29 Mar 2018 13:24:47 -0400 Subject: [PATCH] Ticket 48184 - revert previous patch around unuc-stans shutdown crash https://pagure.io/389-ds-base/issue/48184 --- ldap/servers/slapd/conntable.c | 13 -------- ldap/servers/slapd/daemon.c | 76 +++++++++++++++++------------------------- ldap/servers/slapd/fe.h | 1 - ldap/servers/slapd/slap.h | 1 - 4 files changed, 30 insertions(+), 61 deletions(-) diff --git a/ldap/servers/slapd/conntable.c b/ldap/servers/slapd/conntable.c index f2f763dfa..7c57b47cd 100644 --- a/ldap/servers/slapd/conntable.c +++ b/ldap/servers/slapd/conntable.c @@ -91,19 +91,6 @@ connection_table_abandon_all_operations(Connection_Table *ct) } } -void -connection_table_disconnect_all(Connection_Table *ct) -{ - for (size_t i = 0; i < ct->size; i++) { - if (ct->c[i].c_mutex) { - Connection *c = &(ct->c[i]); - PR_EnterMonitor(c->c_mutex); - disconnect_server_nomutex(c, c->c_connid, -1, SLAPD_DISCONNECT_ABORT, ECANCELED); - PR_ExitMonitor(c->c_mutex); - } - } -} - /* Given a file descriptor for a socket, this function will return * a slot in the connection table to use. * diff --git a/ldap/servers/slapd/daemon.c b/ldap/servers/slapd/daemon.c index c245a4d4e..fcc461a90 100644 --- a/ldap/servers/slapd/daemon.c +++ b/ldap/servers/slapd/daemon.c @@ -1176,30 +1176,6 @@ slapd_daemon(daemon_ports_t *ports, ns_thrpool_t *tp) housekeeping_stop(); /* Run this after op_thread_cleanup() logged sth */ disk_monitoring_stop(); - /* - * Now that they are abandonded, we need to mark them as done. - * In NS while it's safe to allow excess jobs to be cleaned by - * by the walk and ns_job_done of remaining queued events, the - * issue is that if we allow something to live past this point - * the CT is freed from underneath, and bad things happen (tm). - * - * NOTE: We do this after we stop psearch, because there could - * be a race between flagging the psearch done, and users still - * try to send on the connection. Similar with op_threads. - */ - connection_table_disconnect_all(the_connection_table); - - /* - * WARNING: Normally we should close the tp in main - * but because of issues in the current connection design - * we need to close it here to guarantee events won't fire! - * - * All the connection close jobs "should" complete before - * shutdown at least. - */ - ns_thrpool_shutdown(tp); - ns_thrpool_wait(tp); - threads = g_get_active_threadcnt(); if (threads > 0) { slapi_log_err(SLAPI_LOG_INFO, "slapd_daemon", @@ -1652,18 +1628,25 @@ ns_handle_closure(struct ns_job_t *job) Connection *c = (Connection *)ns_job_get_data(job); int do_yield = 0; +/* this function must be called from the event loop thread */ +#ifdef DEBUG + PR_ASSERT(0 == NS_JOB_IS_THREAD(ns_job_get_type(job))); +#else + /* This doesn't actually confirm it's in the event loop thread, but it's a start */ + if (NS_JOB_IS_THREAD(ns_job_get_type(job)) != 0) { + slapi_log_err(SLAPI_LOG_ERR, "ns_handle_closure", "Attempt to close outside of event loop thread %" PRIu64 " for fd=%d\n", + c->c_connid, c->c_sd); + return; + } +#endif + PR_EnterMonitor(c->c_mutex); - /* Assert we really have the right job state. */ - PR_ASSERT(job == c->c_job); connection_release_nolock_ext(c, 1); /* release ref acquired for event framework */ PR_ASSERT(c->c_ns_close_jobs == 1); /* should be exactly 1 active close job - this one */ c->c_ns_close_jobs--; /* this job is processing closure */ - /* Because handle closure will add a new job, we need to detach our current one. */ - c->c_job = NULL; do_yield = ns_handle_closure_nomutex(c); PR_ExitMonitor(c->c_mutex); - /* Remove this task now. */ ns_job_done(job); if (do_yield) { /* closure not done - another reference still outstanding */ @@ -1686,14 +1669,6 @@ ns_connection_post_io_or_closing(Connection *conn) return; } - /* - * Cancel any existing ns jobs we have registered. - */ - if (conn->c_job != NULL) { - ns_job_done(conn->c_job); - conn->c_job = NULL; - } - if (CONN_NEEDS_CLOSING(conn)) { /* there should only ever be 0 or 1 active closure jobs */ PR_ASSERT((conn->c_ns_close_jobs == 0) || (conn->c_ns_close_jobs == 1)); @@ -1703,10 +1678,13 @@ ns_connection_post_io_or_closing(Connection *conn) conn->c_connid, conn->c_sd); return; } else { + /* just make sure we schedule the event to be closed in a timely manner */ + tv.tv_sec = 0; + tv.tv_usec = slapd_wakeup_timer * 1000; conn->c_ns_close_jobs++; /* now 1 active closure job */ connection_acquire_nolock_ext(conn, 1 /* allow acquire even when closing */); /* event framework now has a reference */ - /* Close the job asynchronously. Why? */ - ns_result_t job_result = ns_add_job(conn->c_tp, NS_JOB_TIMER, ns_handle_closure, conn, &(conn->c_job)); + ns_result_t job_result = ns_add_timeout_job(conn->c_tp, &tv, NS_JOB_TIMER, + ns_handle_closure, conn, NULL); if (job_result != NS_SUCCESS) { if (job_result == NS_SHUTDOWN) { slapi_log_err(SLAPI_LOG_INFO, "ns_connection_post_io_or_closing", "post closure job " @@ -1750,7 +1728,7 @@ ns_connection_post_io_or_closing(Connection *conn) #endif ns_result_t job_result = ns_add_io_timeout_job(conn->c_tp, conn->c_prfd, &tv, NS_JOB_READ | NS_JOB_PRESERVE_FD, - ns_handle_pr_read_ready, conn, &(conn->c_job)); + ns_handle_pr_read_ready, conn, NULL); if (job_result != NS_SUCCESS) { if (job_result == NS_SHUTDOWN) { slapi_log_err(SLAPI_LOG_INFO, "ns_connection_post_io_or_closing", "post I/O job for " @@ -1779,12 +1757,19 @@ ns_handle_pr_read_ready(struct ns_job_t *job) int maxthreads = config_get_maxthreadsperconn(); Connection *c = (Connection *)ns_job_get_data(job); - PR_EnterMonitor(c->c_mutex); - /* Assert we really have the right job state. */ - PR_ASSERT(job == c->c_job); +/* this function must be called from the event loop thread */ +#ifdef DEBUG + PR_ASSERT(0 == NS_JOB_IS_THREAD(ns_job_get_type(job))); +#else + /* This doesn't actually confirm it's in the event loop thread, but it's a start */ + if (NS_JOB_IS_THREAD(ns_job_get_type(job)) != 0) { + slapi_log_err(SLAPI_LOG_ERR, "ns_handle_pr_read_ready", "Attempt to handle read ready outside of event loop thread %" PRIu64 " for fd=%d\n", + c->c_connid, c->c_sd); + return; + } +#endif - /* On all code paths we remove the job, so set it null now */ - c->c_job = NULL; + PR_EnterMonitor(c->c_mutex); slapi_log_err(SLAPI_LOG_CONNS, "ns_handle_pr_read_ready", "activity on conn %" PRIu64 " for fd=%d\n", c->c_connid, c->c_sd); @@ -1844,7 +1829,6 @@ ns_handle_pr_read_ready(struct ns_job_t *job) slapi_log_err(SLAPI_LOG_CONNS, "ns_handle_pr_read_ready", "queued conn %" PRIu64 " for fd=%d\n", c->c_connid, c->c_sd); } - /* Since we call done on the job, we need to remove it here. */ PR_ExitMonitor(c->c_mutex); ns_job_done(job); return; diff --git a/ldap/servers/slapd/fe.h b/ldap/servers/slapd/fe.h index f47bb6145..4d25a9fb8 100644 --- a/ldap/servers/slapd/fe.h +++ b/ldap/servers/slapd/fe.h @@ -100,7 +100,6 @@ extern Connection_Table *the_connection_table; /* JCM - Exported from globals.c Connection_Table *connection_table_new(int table_size); void connection_table_free(Connection_Table *ct); void connection_table_abandon_all_operations(Connection_Table *ct); -void connection_table_disconnect_all(Connection_Table *ct); Connection *connection_table_get_connection(Connection_Table *ct, int sd); int connection_table_move_connection_out_of_active_list(Connection_Table *ct, Connection *c); void connection_table_move_connection_on_to_active_list(Connection_Table *ct, Connection *c); diff --git a/ldap/servers/slapd/slap.h b/ldap/servers/slapd/slap.h index 443d90094..9b10aa19e 100644 --- a/ldap/servers/slapd/slap.h +++ b/ldap/servers/slapd/slap.h @@ -1651,7 +1651,6 @@ typedef struct conn void *c_io_layer_cb_data; /* callback data */ struct connection_table *c_ct; /* connection table that this connection belongs to */ ns_thrpool_t *c_tp; /* thread pool for this connection */ - struct ns_job_t *c_job; /* If it exists, the current ns_job_t */ int c_ns_close_jobs; /* number of current close jobs */ char *c_ipaddr; /* ip address str - used by monitor */ } Connection; -- 2.13.6