From d807dba09ef46a3ec070c946d12e299c7de69f7c Mon Sep 17 00:00:00 2001 From: Art Cancro Date: Mon, 20 Sep 2010 13:10:19 -0400 Subject: [PATCH] CtdlThreadSchedule() considered harmful. Eliminate. Replaced all use of it with either dedicated threads or EVT_TIMER hooks. --- citadel/context.c | 1 + citadel/include/ctdl_module.h | 1 - citadel/modules/network/serv_network.c | 48 ++++++++-------- citadel/modules/rssclient/serv_rssclient.c | 8 +-- citadel/modules/smtp/serv_smtp.c | 66 ++++++---------------- citadel/threads.c | 62 -------------------- 6 files changed, 42 insertions(+), 144 deletions(-) diff --git a/citadel/context.c b/citadel/context.c index aa6033c80..922f0675d 100644 --- a/citadel/context.c +++ b/citadel/context.c @@ -437,6 +437,7 @@ void CtdlFillSystemContext(CitContext *context, char *name) strcat (sysname, name); len = cutuserkey(sysname); memcpy(context->curr_user, sysname, len + 1); + context->client_socket = (-1); /* internal_create_user has the side effect of loading the user regardless of wether they * already existed or needed to be created diff --git a/citadel/include/ctdl_module.h b/citadel/include/ctdl_module.h index f1ee1a5f9..c0a13c89d 100644 --- a/citadel/include/ctdl_module.h +++ b/citadel/include/ctdl_module.h @@ -160,7 +160,6 @@ void CtdlModuleStartCryptoMsgs(char *ok_response, char *nosup_response, char *er * Citadel Threads API */ struct CtdlThreadNode *CtdlThreadCreate(char *name, long flags, void *(*thread_func) (void *arg), void *args); -struct CtdlThreadNode *CtdlThreadSchedule(char *name, long flags, void *(*thread_func) (void *arg), void *args, time_t when); void CtdlThreadSleep(int secs); void CtdlThreadStop(struct CtdlThreadNode *thread); int CtdlThreadCheckStop(void); diff --git a/citadel/modules/network/serv_network.c b/citadel/modules/network/serv_network.c index 840fb68e0..c70929542 100644 --- a/citadel/modules/network/serv_network.c +++ b/citadel/modules/network/serv_network.c @@ -2214,15 +2214,10 @@ void create_spool_dirs(void) { * * Run through the rooms doing various types of network stuff. */ -void *network_do_queue(void *args) { +void network_do_queue(void) { static time_t last_run = 0L; struct RoomProcList *ptr; int full_processing = 1; - struct CitContext networkerCC; - - /* Give the networker its own private CitContext */ - CtdlFillSystemContext(&networkerCC, "network"); - citthread_setspecific(MyConKey, (void *)&networkerCC ); /* * Run the full set of processing tasks no more frequently @@ -2242,8 +2237,7 @@ void *network_do_queue(void *args) { * with a static variable instead. */ if (doing_queue) { - CtdlClearSystemContext(); - return NULL; + return; } doing_queue = 1; @@ -2318,20 +2312,6 @@ void *network_do_queue(void *args) { } doing_queue = 0; - - /* Reschedule this task to happen again periodically, unless the thread system indicates - * that the server is shutting down. - */ - if (!CtdlThreadCheckStop()) { - CtdlThreadSchedule("IGnet Network", CTDLTHREAD_BIGSTACK, - network_do_queue, NULL, time(NULL) + 60 - ); - } - else { - CtdlLogPrintf(CTDL_DEBUG, "network: Task STOPPED.\n"); - } - CtdlClearSystemContext(); - return NULL; } @@ -2400,6 +2380,24 @@ int network_room_handler (struct ctdlroom *room) return 0; } +void *ignet_thread(void *arg) { + struct CitContext ignet_thread_CC; + + CtdlLogPrintf(CTDL_DEBUG, "ignet_thread() initializing\n"); + CtdlFillSystemContext(&ignet_thread_CC, "IGnet Queue"); + citthread_setspecific(MyConKey, (void *)&ignet_thread_CC); + + while (!CtdlThreadCheckStop()) { + network_do_queue(); + CtdlThreadSleep(60); + } + + CtdlClearSystemContext(); + return(NULL); +} + + + /* * Module entry point @@ -2413,11 +2411,9 @@ CTDL_MODULE_INIT(network) CtdlRegisterProtoHook(cmd_snet, "SNET", "Set network config"); CtdlRegisterProtoHook(cmd_netp, "NETP", "Identify as network poller"); CtdlRegisterProtoHook(cmd_nsyn, "NSYN", "Synchronize room to node"); - CtdlRegisterRoomHook(network_room_handler); + CtdlRegisterRoomHook(network_room_handler); CtdlRegisterCleanupHook(destroy_network_queue_room); + CtdlThreadCreate("SMTP Send", CTDLTHREAD_BIGSTACK, ignet_thread, NULL); } - else - CtdlThreadSchedule("IGnet Network", CTDLTHREAD_BIGSTACK, network_do_queue, NULL, 0); - /* return our Subversion id for the Log */ return "network"; } diff --git a/citadel/modules/rssclient/serv_rssclient.c b/citadel/modules/rssclient/serv_rssclient.c index f59571db1..049f8801c 100644 --- a/citadel/modules/rssclient/serv_rssclient.c +++ b/citadel/modules/rssclient/serv_rssclient.c @@ -1265,7 +1265,7 @@ void rssclient_scan_room(struct ctdlroom *qrbuf, void *data) /* * Scan for rooms that have RSS client requests configured */ -void *rssclient_scan(void *args) { +void rssclient_scan(void *args) { static time_t last_run = 0L; static int doing_rssclient = 0; rssnetcfg *rptr = NULL; @@ -1298,10 +1298,6 @@ void *rssclient_scan(void *args) { CtdlLogPrintf(CTDL_DEBUG, "rssclient ended\n"); last_run = time(NULL); doing_rssclient = 0; - if (!CtdlThreadCheckStop()) - CtdlThreadSchedule ("RSS Client", CTDLTHREAD_BIGSTACK, rssclient_scan, NULL, last_run + config.c_net_freq); - else - CtdlLogPrintf(CTDL_DEBUG, "rssclient: Task STOPPED.\n"); CtdlClearSystemContext(); return NULL; } @@ -1312,7 +1308,7 @@ CTDL_MODULE_INIT(rssclient) if (threading) { CtdlLogPrintf(CTDL_INFO, "%s\n", curl_version()); - CtdlThreadSchedule ("RSS Client", CTDLTHREAD_BIGSTACK, rssclient_scan, NULL, 0); + CtdlRegisterSessionHook(rssclient_scan, EVT_TIMER); } StartHandlers = NewHash(1, NULL); diff --git a/citadel/modules/smtp/serv_smtp.c b/citadel/modules/smtp/serv_smtp.c index 69f12fb03..ab01bbd7d 100644 --- a/citadel/modules/smtp/serv_smtp.c +++ b/citadel/modules/smtp/serv_smtp.c @@ -119,8 +119,6 @@ enum { /* Command states for login authentication */ int run_queue_now = 0; /* Set to 1 to ignore SMTP send retry times */ -citthread_mutex_t smtp_send_lock; - /*****************************************************************************/ /* SMTP SERVER (INBOUND) STUFF */ @@ -1732,27 +1730,31 @@ void smtp_do_procmsg(long msgnum, void *userdata) { /* - * smtp_do_queue() + * smtp_queue_thread() * * Run through the queue sending out messages. */ -void *smtp_do_queue(void *arg) { +void *smtp_queue_thread(void *arg) { int num_processed = 0; struct CitContext smtp_queue_CC; CtdlFillSystemContext(&smtp_queue_CC, "SMTP Send"); - citthread_setspecific(MyConKey, (void *)&smtp_queue_CC ); - CtdlLogPrintf(CTDL_INFO, "SMTP client: processing outbound queue\n"); + citthread_setspecific(MyConKey, (void *)&smtp_queue_CC); + CtdlLogPrintf(CTDL_DEBUG, "smtp_queue_thread() initializing\n"); - if (CtdlGetRoom(&CC->room, SMTP_SPOOLOUT_ROOM) != 0) { - CtdlLogPrintf(CTDL_ERR, "Cannot find room <%s>\n", SMTP_SPOOLOUT_ROOM); - } - else { - num_processed = CtdlForEachMessage(MSGS_ALL, 0L, NULL, SPOOLMIME, NULL, smtp_do_procmsg, NULL); - } + while (!CtdlThreadCheckStop()) { + + CtdlLogPrintf(CTDL_INFO, "SMTP client: processing outbound queue\n"); - citthread_mutex_unlock (&smtp_send_lock); - CtdlLogPrintf(CTDL_INFO, "SMTP client: queue run completed; %d messages processed\n", num_processed); + if (CtdlGetRoom(&CC->room, SMTP_SPOOLOUT_ROOM) != 0) { + CtdlLogPrintf(CTDL_ERR, "Cannot find room <%s>\n", SMTP_SPOOLOUT_ROOM); + } + else { + num_processed = CtdlForEachMessage(MSGS_ALL, 0L, NULL, SPOOLMIME, NULL, smtp_do_procmsg, NULL); + } + CtdlLogPrintf(CTDL_INFO, "SMTP client: queue run completed; %d messages processed\n", num_processed); + CtdlThreadSleep(60); + } CtdlClearSystemContext(); return(NULL); @@ -1760,38 +1762,6 @@ void *smtp_do_queue(void *arg) { -/* - * smtp_queue_thread - * - * Create a thread to run the SMTP queue - * - * This was created as a response to a situation seen on Uncensored where a bad remote was holding - * up SMTP sending for long times. - * Converting to a thread does not fix the problem caused by the bad remote but it does prevent - * the SMTP sending from stopping housekeeping and the EVT_TIMER event system which in turn prevented - * other things from happening. - */ -void smtp_queue_thread (void) -{ - if (citthread_mutex_trylock (&smtp_send_lock)) { - CtdlLogPrintf(CTDL_DEBUG, "SMTP queue run already in progress\n"); - } - else { - CtdlThreadCreate("SMTP Send", CTDLTHREAD_BIGSTACK, smtp_do_queue, NULL); - } -} - - - -void smtp_server_going_down (void) -{ - CtdlLogPrintf(CTDL_DEBUG, "SMTP module clean up for shutdown.\n"); - - citthread_mutex_destroy (&smtp_send_lock); -} - - - /*****************************************************************************/ /* SMTP UTILITY COMMANDS */ /*****************************************************************************/ @@ -1924,11 +1894,9 @@ CTDL_MODULE_INIT(smtp) CitadelServiceSMTP_LMTP_UNF); smtp_init_spoolout(); - CtdlRegisterSessionHook(smtp_queue_thread, EVT_TIMER); CtdlRegisterSessionHook(smtp_cleanup_function, EVT_STOP); CtdlRegisterProtoHook(cmd_smtp, "SMTP", "SMTP utility commands"); - CtdlRegisterCleanupHook (smtp_server_going_down); - citthread_mutex_init (&smtp_send_lock, NULL); + CtdlThreadCreate("SMTP Send", CTDLTHREAD_BIGSTACK, smtp_queue_thread, NULL); } /* return our Subversion id for the Log */ diff --git a/citadel/threads.c b/citadel/threads.c index 96d3e4c04..ec64ceaef 100644 --- a/citadel/threads.c +++ b/citadel/threads.c @@ -1042,68 +1042,6 @@ CtdlThreadNode *CtdlThreadCreate(char *name, long flags, void *(*thread_func) (v -/* - * Internal function to schedule a thread. - * Must be called from within a S_THREAD_LIST critical section - */ -CtdlThreadNode *CtdlThreadSchedule(char *name, long flags, void *(*thread_func) (void *arg), void *args, time_t when) -{ - CtdlThreadNode *this_thread; - - if (num_threads >= 32767) - { - CtdlLogPrintf(CTDL_EMERG, "Thread system. Thread list full.\n"); - return NULL; - } - - this_thread = malloc(sizeof(CtdlThreadNode)); - if (this_thread == NULL) { - CtdlLogPrintf(CTDL_EMERG, "Thread system, can't allocate CtdlThreadNode, exiting\n"); - return NULL; - } - /* Initialise the thread structure */ - if (ctdl_internal_init_thread_struct(this_thread, flags) == NULL) - { - free(this_thread); - CtdlLogPrintf(CTDL_EMERG, "Thread system, can't initialise CtdlThreadNode, exiting\n"); - return NULL; - } - - /* - * If we got here we are going to create the thread so we must initilise the structure - * first because most implimentations of threading can't create it in a stopped state - * and it might want to do things with its structure that aren't initialised otherwise. - */ - if(name) - { - this_thread->name = name; - } - else - { - this_thread->name = "Un-named Thread"; - } - - this_thread->flags = flags; - this_thread->thread_func = thread_func; - this_thread->user_args = args; - - /* - * When to start this thread - */ - this_thread->when = when; - - begin_critical_section(S_SCHEDULE_LIST); - this_thread->next = CtdlThreadSchedList; - CtdlThreadSchedList = this_thread; - if (this_thread->next) - this_thread->next->prev = this_thread; - end_critical_section(S_SCHEDULE_LIST); - - return this_thread; -} - - - CtdlThreadNode *ctdl_thread_internal_start_scheduled (CtdlThreadNode *this_thread) { int ret = 0; -- 2.30.2