Re: [Mono-dev] Thread shutdown hook patch
All I can add is that the logging statistical profiler needs it. As it is in svn it does not crash anymore, but it terminates when the main thread ends. So, for instance, it is totally unable to profile "xsp2 --nonstop" (while with this patch, and using the callback, is works fine). Thanks! Massi ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] Thread shutdown hook patch
On Wed, Feb 11, 2009 at 12:55 PM, Paolo Molaro wrote:
> On 02/09/09 Rodrigo Kumpera wrote:
> > > The attached patch adds a new hook to allow threads to shutdown after
> the
> > > GC finalizer has finished.
> > > The motivation for it is to improve profiler's reliability at shutdown
> > > time.
> > >
> > > The new callback notifies the thread when regular shutdown starts and
> gives
> > > it a change to not
> > > finish at this time.
> > >
> > > Later on the same callback is used to notify the thread that the last
> stage
> > > in the shutdown sequence
> > > has been reached and it must shutdown.
> > >
> > > The callback is per-thread as I don't want to have tools like the
> profiler
> > > messing up with embedded users.
>
> I think this instroduces too much complexity: since the API is public
> we'd need to end up maintaining the semantics as they happen to be now.
> It would be much clearer to have a flag on the thread that behaves
> similarly to critical finalizers: the thread that has it set will be
> destroyed as late as possible on shutdown.
> As for the callback, the existing mono_profiler_thread_end () should be
> enough: if it isn't we need to discuss how we can fix that instead of
> adding yet another callback.
>
> lupus
>
>
Let's see, to make what you propose work we would have to do the following
changes:
1) Introduce a flag to signal critical threads, which are only shutdown at
the last possible opportunity;
Together with a mono_thread_set_critical (MonoThread*) public API to set
it, or course.
2) Move the profiler shutdown to mono_runtime_cleanup so it happens before
we wait for critical threads;
This is required because the thread shutdown code has no means to ask
unmanaged code to shutdown.
A critical thread is one that
mono_profiler_thread_end doesn't play any role for this issue because it is
called once the thread has finished or detached,
which is not the issue here.
With this patch the code in the profiler gets simpler and more natural.
Everything is done from the profiler shutdown hook and
not piece by piece scattered around.
There is one issue with this patch, there is no unmanaged mechanism to
inform a thread that it must shutdown. Right now this isn't
an issue because the only user will be the profiller and it does all it's
cleanup from the shutdown hook. But it might not be the case
for embedders attaching critical threads.
I don't think this is an usable API without a well defined mechanism to let
embedders/tools know when it's time for a critical thread
to begin to shutdown.
The attached patch implements this proposal, please review.
Cheers,
Rodrigo
Index: metadata/appdomain.c
===
--- metadata/appdomain.c (revision 126598)
+++ metadata/appdomain.c (working copy)
@@ -302,6 +302,10 @@
/* This ends up calling any pending pending (for at most 2 seconds) */
mono_gc_cleanup ();
+ mono_profiler_shutdown ();
+
+ mono_thread_wait_critical_threads ();
+
mono_thread_cleanup ();
#ifndef DISABLE_SOCKETS
Index: metadata/threads.c
===
--- metadata/threads.c (revision 126598)
+++ metadata/threads.c (working copy)
@@ -93,6 +93,10 @@
MonoHazardousFreeFunc free_func;
} DelayedFreeItem;
+enum {
+ MONO_THREAD_CRITICAL = 0x1
+};
+
/* Number of cached culture objects in the MonoThread->cached_culture_info array
* (per-type): we use the first NUM entries for CultureInfo and the last for
* UICultureInfo. So the size of the array is really NUM_CACHED_CULTURES * 2.
@@ -167,6 +171,7 @@
static gboolean mono_thread_resume (MonoThread* thread);
static void mono_thread_start (MonoThread *thread);
static void signal_thread_state_change (MonoThread *thread);
+static gboolean mono_thread_is_critical (MonoThread *thread);
/* Spin lock for InterlockedXXX 64 bit functions */
#define mono_interlocked_lock() EnterCriticalSection (&interlocked_mutex)
@@ -2534,6 +2539,11 @@
return;
}
+ if (mono_thread_is_critical (thread)) {
+ THREAD_DEBUG (g_message ("%s: ignoring critical thread %"G_GSIZE_FORMAT, __func__, (gsize)thread->tid));
+ return;
+ }
+
handle = OpenThread (THREAD_ALL_ACCESS, TRUE, thread->tid);
if (handle == NULL) {
THREAD_DEBUG (g_message ("%s: ignoring unopenable thread %"G_GSIZE_FORMAT, __func__, (gsize)thread->tid));
@@ -2571,7 +2581,7 @@
return FALSE;
/* The finalizer thread is not a background thread */
- if (thread->tid != self && (thread->state & ThreadState_Background) != 0) {
+ if (thread->tid != self && (thread->state & ThreadState_Background) != 0 && !mono_thread_is_critical (thread)) {
handle = OpenThread (THREAD_ALL_ACCESS, TRUE, thread->tid);
if (handle == NULL)
@@ -2768,6 +2778,9 @@
(thread->state & ThreadState_Stopped) != 0)
return;
+ if (mono_thread_is_critical (thread))
+ return;
+
if (wait->numtid);
if (handle == NULL)
@@ -3777,3 +3790,64 @@
return ret;
}
+
+/*
+
Re: [Mono-dev] Thread shutdown hook patch
On 02/09/09 Rodrigo Kumpera wrote: > > The attached patch adds a new hook to allow threads to shutdown after the > > GC finalizer has finished. > > The motivation for it is to improve profiler's reliability at shutdown > > time. > > > > The new callback notifies the thread when regular shutdown starts and gives > > it a change to not > > finish at this time. > > > > Later on the same callback is used to notify the thread that the last stage > > in the shutdown sequence > > has been reached and it must shutdown. > > > > The callback is per-thread as I don't want to have tools like the profiler > > messing up with embedded users. I think this instroduces too much complexity: since the API is public we'd need to end up maintaining the semantics as they happen to be now. It would be much clearer to have a flag on the thread that behaves similarly to critical finalizers: the thread that has it set will be destroyed as late as possible on shutdown. As for the callback, the existing mono_profiler_thread_end () should be enough: if it isn't we need to discuss how we can fix that instead of adding yet another callback. lupus -- - lu...@debian.org debian/rules lu...@ximian.com Monkeys do it better ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] Thread shutdown hook patch
And updated version without some useless printf removed.
On Mon, Feb 9, 2009 at 4:22 PM, Rodrigo Kumpera wrote:
> Hey,
>
> The attached patch adds a new hook to allow threads to shutdown after the
> GC finalizer has finished.
> The motivation for it is to improve profiler's reliability at shutdown
> time.
>
> The new callback notifies the thread when regular shutdown starts and gives
> it a change to not
> finish at this time.
>
> Later on the same callback is used to notify the thread that the last stage
> in the shutdown sequence
> has been reached and it must shutdown.
>
> The callback is per-thread as I don't want to have tools like the profiler
> messing up with embedded users.
>
> Please review.
>
>
> Cheers,
> Rodrigo
>
>
Index: appdomain.c
===
--- appdomain.c (revision 126331)
+++ appdomain.c (working copy)
@@ -302,6 +302,8 @@
/* This ends up calling any pending pending (for at most 2 seconds) */
mono_gc_cleanup ();
+ mono_thread_wait_last_threads ();
+
mono_thread_cleanup ();
#ifndef DISABLE_SOCKETS
Index: threads.c
===
--- threads.c (revision 126331)
+++ threads.c (working copy)
@@ -2393,6 +2393,12 @@
thread->manage_callback = func;
}
+void
+mono_thread_set_shutdown_callback (MonoThread *thread, MonoThreadShutdownCallback func)
+{
+ thread->shutdown_callback = func;
+}
+
void mono_threads_install_notify_pending_exc (MonoThreadNotifyPendingExcFunc func)
{
mono_thread_notify_pending_exc_fn = func;
@@ -2570,6 +2576,9 @@
if (wait->num >= MAXIMUM_WAIT_OBJECTS)
return FALSE;
+ if (thread->shutdown_callback && !thread->shutdown_callback (thread, ShutdownRegular))
+ return FALSE;
+
/* The finalizer thread is not a background thread */
if (thread->tid != self && (thread->state & ThreadState_Background) != 0) {
@@ -2729,6 +2738,76 @@
g_free (wait);
}
+static void
+build_last_stage_wait_list (gpointer key, gpointer value, gpointer user)
+{
+ struct wait_data *wait=(struct wait_data *)user;
+ gsize self = GetCurrentThreadId ();
+ MonoThread *thread = (MonoThread *) value;
+ HANDLE handle;
+
+ if (wait->num >= MAXIMUM_WAIT_OBJECTS)
+ return;
+
+ if ((thread->state & ThreadState_Suspended) != 0 ||
+ (thread->state & ThreadState_Stopped) != 0)
+ return;
+
+ if (thread->tid != self) {
+ /*only threads with shutdown_callback might have survived so far.*/
+ if (thread->shutdown_callback)
+ g_assert (thread->shutdown_callback (thread, ShutdownLastOpportunity));
+
+ handle = OpenThread (THREAD_ALL_ACCESS, TRUE, thread->tid);
+ if (handle == NULL)
+ return;
+
+ wait->handles[wait->num]=thread->handle;
+ wait->threads[wait->num]=thread;
+ wait->num++;
+
+ THREAD_DEBUG (g_print ("%s: Last chance abort id: %x\n", __func__, (gsize)thread->tid));
+ mono_thread_stop (thread);
+ }
+}
+
+void
+mono_thread_wait_last_threads (void)
+{
+ struct wait_data *wait = g_new0 (struct wait_data, 1);
+
+ /* join each thread that's still running */
+ THREAD_DEBUG (g_message ("%s: Joining threads after GC finalization...", __func__));
+
+ do {
+ mono_threads_lock ();
+ THREAD_DEBUG (g_message ("%s: There are %d threads to join", __func__, mono_g_hash_table_size (threads));
+ mono_g_hash_table_foreach (threads, print_tids, NULL));
+
+ wait->num = 0;
+ mono_g_hash_table_foreach (threads, build_last_stage_wait_list, wait);
+
+ mono_threads_unlock ();
+
+ THREAD_DEBUG (g_message ("%s: wait->num is now %d", __func__, wait->num));
+
+ if(wait->num > 0)
+ wait_for_tids (wait, INFINITE);
+ THREAD_DEBUG (g_message ("%s: I have %d threads after waiting.", __func__, wait->num));
+ } while(wait->num > 0);
+
+ /*
+ * give the subthreads a chance to really quit (this is mainly needed
+ * to get correct user and system times from getrusage/wait/time(1)).
+ * This could be removed if we avoid pthread_detach() and use pthread_join().
+ */
+#ifndef PLATFORM_WIN32
+ sched_yield ();
+#endif
+
+ g_free (wait);
+}
+
static void terminate_thread (gpointer key, gpointer value, gpointer user)
{
MonoThread *thread=(MonoThread *)value;
@@ -2768,6 +2847,9 @@
(thread->state & ThreadState_Stopped) != 0)
return;
+ if (thread->shutdown_callback && !thread->shutdown_callback (thread, ShutdownRegular))
+ return;
+
if (wait->numtid);
if (handle == NULL)
Index: threads.h
===
--- threads.h (revision 126331)
+++ threads.h (working copy)
@@ -19,9 +19,34 @@
typedef void (*MonoThreadCleanupFunc) (MonoThread* thread);
+typedef enum {
+ ShutdownRegular = 1, /*Once regular shutdown has begun*/
+ ShutdownLastOpportunity = 99 /*After the finalizer thread has finished*/
+} MonoShutdownSequence;
+
/* This callback should return TRUE if the runtime must wait for the thread, FALSE otherwise */
typedef gboolean (*MonoThreadManageCallback) (MonoThread* thread);
+/*
+This callback allows to interact
