From c753a2a3d836b70fdfe96c456e383388fa4f3e2a Mon Sep 17 00:00:00 2001 From: 5 Date: Tue, 25 Sep 2001 22:09:04 +0000 Subject: Fix for !threads enabled not ccompiling. (camel_operation_ref): Assert 2001-09-25 * camel-operation.c (camel_operation_unref): Fix for !threads enabled not ccompiling. (camel_operation_ref): Assert refcount > 0. (struct _CamelOperation): Removed the lock. On further investigation, I dont think this will always work, the registration operations assume that a lookup in the operation_active table will return a ref, that will remain valid until we ref it, which needn't be the case. So now i'm using a single global lock, since we'd need to do that for unref anyway, and every operation is fast & memory-bound. Changed all the code to handle this. (camel_operation_progress_count): Since the code is identical, just call progress() for now. (camel_operation_register): No longer refcount, use unref to check/clear the active table. (camel_operation_unregister): Same here. (camel_operation_unref): Check if operation is in active table, if so, warn, remove. svn path=/trunk/; revision=13125 --- camel/ChangeLog | 21 ++++ camel/camel-operation.c | 302 +++++++++++++++++++++++------------------------- camel/camel-operation.h | 2 + 3 files changed, 166 insertions(+), 159 deletions(-) diff --git a/camel/ChangeLog b/camel/ChangeLog index b98ca0888a..97ccb5bfd7 100644 --- a/camel/ChangeLog +++ b/camel/ChangeLog @@ -1,3 +1,24 @@ +2001-09-25 + + * camel-operation.c (camel_operation_unref): Fix for !threads + enabled not ccompiling. + (camel_operation_ref): Assert refcount > 0. + (struct _CamelOperation): Removed the lock. On further + investigation, I dont think this will always work, the + registration operations assume that a lookup in the + operation_active table will return a ref, that will remain valid + until we ref it, which needn't be the case. So now i'm using a + single global lock, since we'd need to do that for unref anyway, + and every operation is fast & memory-bound. Changed all the code + to handle this. + (camel_operation_progress_count): Since the code is identical, + just call progress() for now. + (camel_operation_register): No longer refcount, use unref to + check/clear the active table. + (camel_operation_unregister): Same here. + (camel_operation_unref): Check if operation is in active table, if + so, warn, remove. + 2001-09-25 Dan Winship * camel-tcp-stream-openssl.c (my_SSL_read, my_SSL_write): call diff --git a/camel/camel-operation.c b/camel/camel-operation.c index 708259ff32..42aa448820 100644 --- a/camel/camel-operation.c +++ b/camel/camel-operation.c @@ -42,7 +42,6 @@ struct _CamelOperation { #ifdef ENABLE_THREADS EMsgPort *cancel_port; int cancel_fd; - pthread_mutex_t lock; #endif }; @@ -50,14 +49,10 @@ struct _CamelOperation { #define CAMEL_OPERATION_TRANSIENT (1<<1) #ifdef ENABLE_THREADS -#define CAMEL_OPERATION_LOCK(cc) pthread_mutex_lock(&cc->lock) -#define CAMEL_OPERATION_UNLOCK(cc) pthread_mutex_unlock(&cc->lock) #define CAMEL_ACTIVE_LOCK() pthread_mutex_lock(&operation_active_lock) #define CAMEL_ACTIVE_UNLOCK() pthread_mutex_unlock(&operation_active_lock) static pthread_mutex_t operation_active_lock = PTHREAD_MUTEX_INITIALIZER; #else -#define CAMEL_OPERATION_LOCK(cc) -#define CAMEL_OPERATION_UNLOCK(cc) #define CAMEL_ACTIVE_LOCK() #define CAMEL_ACTIVE_UNLOCK() #endif @@ -98,12 +93,27 @@ CamelOperation *camel_operation_new(CamelOperationStatusFunc status, void *statu cc->id = ~0; cc->cancel_port = e_msgport_new(); cc->cancel_fd = e_msgport_fd(cc->cancel_port); - pthread_mutex_init(&cc->lock, NULL); #endif return cc; } +/* return the registered operation, or NULL if none registered */ +/* need to unref when done with it */ +CamelOperation *camel_operation_registered(void) +{ + CamelOperation *cc = NULL; + + CAMEL_ACTIVE_LOCK(); + if (operation_active != NULL) + cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); + g_assert(cc->refcount > 0); + cc->refcount++; + CAMEL_ACTIVE_UNLOCK(); + + return cc; +} + /** * camel_operation_reset: * @cc: @@ -141,9 +151,11 @@ void camel_operation_reset(CamelOperation *cc) **/ void camel_operation_ref(CamelOperation *cc) { - CAMEL_OPERATION_LOCK(cc); + g_assert(cc->refcount > 0); + + CAMEL_ACTIVE_LOCK(); cc->refcount++; - CAMEL_OPERATION_UNLOCK(cc); + CAMEL_ACTIVE_UNLOCK(); } /** @@ -155,15 +167,25 @@ void camel_operation_ref(CamelOperation *cc) void camel_operation_unref(CamelOperation *cc) { GSList *n; -#ifdef ENABLE_THREADS - CamelOperationMsg *msg; + g_assert(cc->refcount > 0); + + CAMEL_ACTIVE_LOCK(); if (cc->refcount == 1) { +#ifdef ENABLE_THREADS + CamelOperationMsg *msg; + while ((msg = (CamelOperationMsg *)e_msgport_get(cc->cancel_port))) g_free(msg); e_msgport_destroy(cc->cancel_port); #endif + + if (cc->id != (~0)) { + g_warning("Unreffing operation status which was still registered: %p\n", cc); + g_hash_table_remove(operation_active, (void *)cc->id); + } + n = cc->status_stack; while (n) { g_warning("Camel operation status stack non empty: %s", (char *)n->data); @@ -174,10 +196,9 @@ void camel_operation_unref(CamelOperation *cc) g_free(cc); } else { - CAMEL_OPERATION_LOCK(cc); cc->refcount--; - CAMEL_OPERATION_UNLOCK(cc); } + CAMEL_ACTIVE_UNLOCK(); } /** @@ -195,13 +216,10 @@ void camel_operation_cancel_block(CamelOperation *cc) if (cc == NULL) cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc) { - CAMEL_OPERATION_LOCK(cc); + if (cc) cc->blocked++; - CAMEL_OPERATION_UNLOCK(cc); - } + CAMEL_ACTIVE_UNLOCK(); } /** @@ -220,20 +238,24 @@ void camel_operation_cancel_unblock(CamelOperation *cc) if (cc == NULL) cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc) { - CAMEL_OPERATION_LOCK(cc); + if (cc) cc->blocked--; - CAMEL_OPERATION_UNLOCK(cc); - } + CAMEL_ACTIVE_UNLOCK(); } static void cancel_thread(void *key, CamelOperation *cc, void *data) { - if (cc) - camel_operation_cancel(cc); + CamelOperationMsg *msg; + + if (cc) { + d(printf("cancelling thread %d\n", cc->id)); + + msg = g_malloc0(sizeof(*msg)); + e_msgport_put(cc->cancel_port, (EMsg *)msg); + cc->flags |= CAMEL_OPERATION_CANCELLED; + } } /** @@ -246,22 +268,22 @@ cancel_thread(void *key, CamelOperation *cc, void *data) void camel_operation_cancel(CamelOperation *cc) { CamelOperationMsg *msg; + + CAMEL_ACTIVE_LOCK(); if (cc == NULL) { if (operation_active) { - CAMEL_ACTIVE_LOCK(); g_hash_table_foreach(operation_active, (GHFunc)cancel_thread, NULL); - CAMEL_ACTIVE_UNLOCK(); } } else if ((cc->flags & CAMEL_OPERATION_CANCELLED) == 0) { d(printf("cancelling thread %d\n", cc->id)); - CAMEL_OPERATION_LOCK(cc); msg = g_malloc0(sizeof(*msg)); e_msgport_put(cc->cancel_port, (EMsg *)msg); cc->flags |= CAMEL_OPERATION_CANCELLED; - CAMEL_OPERATION_UNLOCK(cc); } + + CAMEL_ACTIVE_UNLOCK(); } /** @@ -301,8 +323,6 @@ void camel_operation_register(CamelOperation *cc) d(printf("registering thread %ld for cancellation\n", id)); CAMEL_ACTIVE_UNLOCK(); - - camel_operation_ref(cc); } /** @@ -338,9 +358,6 @@ void camel_operation_unregister(CamelOperation *cc) CAMEL_ACTIVE_UNLOCK(); d({if (cc) printf("unregistering thread %d for cancellation\n", cc->id);}); - - if (cc) - camel_operation_unref(cc); } /** @@ -355,38 +372,32 @@ void camel_operation_unregister(CamelOperation *cc) gboolean camel_operation_cancel_check(CamelOperation *cc) { CamelOperationMsg *msg; + int cancelled; d(printf("checking for cancel in thread %d\n", pthread_self())); - if (cc == NULL) { - if (operation_active) { - CAMEL_ACTIVE_LOCK(); - cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - } - if (cc == NULL) - return FALSE; - } + CAMEL_ACTIVE_LOCK(); - if (cc->blocked > 0) { - d(printf("ahah! cancellation is blocked\n")); - return FALSE; - } + if (cc == NULL && operation_active) + cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - if (cc->flags & CAMEL_OPERATION_CANCELLED) { + if (cc == NULL || cc->blocked > 0) { + d(printf("ahah! cancellation is blocked\n")); + cancelled = FALSE; + } else if (cc->flags & CAMEL_OPERATION_CANCELLED) { d(printf("previously cancelled\n")); - return TRUE; - } - - msg = (CamelOperationMsg *)e_msgport_get(cc->cancel_port); - if (msg) { + cancelled = TRUE; + } else if ((msg = (CamelOperationMsg *)e_msgport_get(cc->cancel_port))) { d(printf("Got cancellation message\n")); - CAMEL_OPERATION_LOCK(cc); + g_free(msg); cc->flags |= CAMEL_OPERATION_CANCELLED; - CAMEL_OPERATION_UNLOCK(cc); - return TRUE; - } - return FALSE; + cancelled = TRUE; + } else + cancelled = FALSE; + + CAMEL_ACTIVE_UNLOCK(); + + return cancelled; } /** @@ -401,15 +412,15 @@ gboolean camel_operation_cancel_check(CamelOperation *cc) **/ int camel_operation_cancel_fd(CamelOperation *cc) { - if (cc == NULL) { - if (operation_active) { - CAMEL_ACTIVE_LOCK(); - cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - } - if (cc == NULL) - return -1; + if (cc == NULL && operation_active) { + CAMEL_ACTIVE_LOCK(); + cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); + CAMEL_ACTIVE_UNLOCK(); } + + if (cc == NULL) + return -1; + if (cc->blocked) return -1; @@ -434,27 +445,30 @@ void camel_operation_start(CamelOperation *cc, char *what, ...) if (operation_active == NULL) return; - if (cc == NULL) { - CAMEL_ACTIVE_LOCK(); + CAMEL_ACTIVE_LOCK(); + + if (cc == NULL) cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc == NULL) - return; - } - if (cc->status == NULL) + if (cc == NULL || cc->status == NULL) { + CAMEL_ACTIVE_UNLOCK(); return; + } va_start(ap, what); msg = g_strdup_vprintf(what, ap); va_end(ap); - cc->status(cc, msg, CAMEL_OPERATION_START, cc->status_data); cc->status_update = 0; s = g_malloc0(sizeof(*s)); s->msg = msg; s->flags = 0; cc->lastreport = s; cc->status_stack = g_slist_prepend(cc->status_stack, s); + + CAMEL_ACTIVE_UNLOCK(); + + cc->status(cc, msg, CAMEL_OPERATION_START, cc->status_data); + d(printf("start '%s'\n", msg, pc)); } @@ -477,22 +491,19 @@ void camel_operation_start_transient(CamelOperation *cc, char *what, ...) if (operation_active == NULL) return; - if (cc == NULL) { - CAMEL_ACTIVE_LOCK(); + CAMEL_ACTIVE_LOCK(); + + if (cc == NULL) cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc == NULL) - return; - } - if (cc->status == NULL) + if (cc == NULL || cc->status == NULL) { + CAMEL_ACTIVE_UNLOCK(); return; + } va_start(ap, what); msg = g_strdup_vprintf(what, ap); va_end(ap); - /* we dont report it yet */ - /*cc->status(cc, msg, CAMEL_OPERATION_START, cc->status_data);*/ cc->status_update = 0; s = g_malloc0(sizeof(*s)); s->msg = msg; @@ -500,6 +511,11 @@ void camel_operation_start_transient(CamelOperation *cc, char *what, ...) s->stamp = stamp(); cc->status_stack = g_slist_prepend(cc->status_stack, s); d(printf("start '%s'\n", msg, pc)); + + CAMEL_ACTIVE_UNLOCK(); + + /* we dont report it yet */ + /*cc->status(cc, msg, CAMEL_OPERATION_START, cc->status_data);*/ } static unsigned int stamp(void) @@ -527,86 +543,46 @@ void camel_operation_progress(CamelOperation *cc, int pc) { unsigned int now; struct _status_stack *s; + char *msg = NULL; if (operation_active == NULL) return; - if (cc == NULL) { - CAMEL_ACTIVE_LOCK(); - cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc == NULL) - return; - } + CAMEL_ACTIVE_LOCK(); - if (cc->status == NULL) - return; + if (cc == NULL) + cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - if (cc->status_stack == NULL) + if (cc == NULL || cc->status == NULL || cc->status_stack == NULL) { + CAMEL_ACTIVE_UNLOCK(); return; + } s = cc->status_stack->data; s->pc = pc; now = stamp(); - if (cc->status_update != now) { - if (s->flags & CAMEL_OPERATION_TRANSIENT) { - if (s->stamp/16 < now/16) { - s->stamp = now; - cc->status(cc, s->msg, pc, cc->status_data); - cc->status_update = now; - cc->lastreport = s; - } - } else { - cc->status(cc, s->msg, pc, cc->status_data); - d(printf("progress '%s' %d %%\n", s->msg, pc)); - s->stamp = cc->status_update = now; - cc->lastreport = s; - } + if (cc->status_update != now + && s->flags & CAMEL_OPERATION_TRANSIENT + && s->stamp/16 > now/16) + cc = NULL; + else { + s->stamp = cc->status_update = now; + cc->lastreport = s; + msg = g_strdup(s->msg); } -} - -void camel_operation_progress_count(CamelOperation *cc, int sofar) -{ - unsigned int now; - struct _status_stack *s; - if (operation_active == NULL) - return; + CAMEL_ACTIVE_UNLOCK(); - if (cc == NULL) { - CAMEL_ACTIVE_LOCK(); - cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc == NULL) - return; + if (cc) { + cc->status(cc, msg, pc, cc->status_data); + g_free(msg); } +} - if (cc->status == NULL) - return; - - if (cc->status_stack == NULL) - return; - - /* FIXME: generate some meaningful pc value */ - s = cc->status_stack->data; - s->pc = sofar; - now = stamp(); - if (cc->status_update != now) { - if (s->flags & CAMEL_OPERATION_TRANSIENT) { - if (s->stamp/16 < now/16) { - s->stamp = now; - cc->status(cc, s->msg, sofar, cc->status_data); - cc->status_update = now; - cc->lastreport = s; - } - } else { - cc->status(cc, s->msg, sofar, cc->status_data); - d(printf("progress '%s' %d done\n", msg, sofar)); - s->stamp = cc->status_update = now; - cc->lastreport = s; - } - } +void camel_operation_progress_count(CamelOperation *cc, int sofar) +{ + camel_operation_progress(cc, sofar); } /** @@ -622,23 +598,21 @@ void camel_operation_end(CamelOperation *cc) { struct _status_stack *s, *p; unsigned int now; + char *msg = NULL; + int pc = 0; if (operation_active == NULL) return; - if (cc == NULL) { - CAMEL_ACTIVE_LOCK(); - cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - CAMEL_ACTIVE_UNLOCK(); - if (cc == NULL) - return; - } + CAMEL_ACTIVE_LOCK(); - if (cc->status == NULL) - return; + if (cc == NULL) + cc = g_hash_table_lookup(operation_active, (void *)pthread_self()); - if (cc->status_stack == NULL) + if (cc == NULL || cc->status == NULL || cc->status_stack == NULL) { + CAMEL_ACTIVE_UNLOCK(); return; + } /* so what we do here is this. If the operation that just * ended was transient, see if we have any other transient @@ -653,23 +627,33 @@ void camel_operation_end(CamelOperation *cc) p = l->data; if (p->flags & CAMEL_OPERATION_TRANSIENT) { if (p->stamp/16 < now/16) { - cc->status(cc, p->msg, p->pc, cc->status_data); + msg = g_strdup(p->msg); + pc = p->pc; cc->lastreport = p; break; } } else { - cc->status(cc, p->msg, p->pc, cc->status_data); + msg = g_strdup(p->msg); + pc = p->pc; cc->lastreport = p; break; } l = l->next; } } + g_free(s->msg); } else { - cc->status(cc, s->msg, CAMEL_OPERATION_END, cc->status_data); + msg = s->msg; + pc = CAMEL_OPERATION_END; cc->lastreport = s; } - g_free(s->msg); g_free(s); cc->status_stack = g_slist_remove_link(cc->status_stack, cc->status_stack); + + CAMEL_ACTIVE_UNLOCK(); + + if (msg) { + cc->status(cc, msg, pc, cc->status_data); + g_free(msg); + } } diff --git a/camel/camel-operation.h b/camel/camel-operation.h index 9dab0b0377..25bd77e408 100644 --- a/camel/camel-operation.h +++ b/camel/camel-operation.h @@ -53,6 +53,8 @@ void camel_operation_cancel_block(CamelOperation *cc); void camel_operation_cancel_unblock(CamelOperation *cc); int camel_operation_cancel_check(CamelOperation *cc); int camel_operation_cancel_fd(CamelOperation *cc); +/* return the registered operation for this thread, if there is one */ +CamelOperation *camel_operation_registered(void); void camel_operation_start(CamelOperation *cc, char *what, ...); void camel_operation_start_transient(CamelOperation *cc, char *what, ...); -- cgit v1.2.3