Merge pull request #3029 from lambdageek/dev/simplify-monoerror-threads
authorAleksey Kliger (λgeek) <akliger@gmail.com>
Tue, 24 May 2016 15:08:01 +0000 (11:08 -0400)
committerAleksey Kliger (λgeek) <akliger@gmail.com>
Tue, 24 May 2016 15:08:01 +0000 (11:08 -0400)
[runtime] Simplify MonoError handling in finalizer creation and in thread objects

mono/metadata/appdomain.c
mono/metadata/cominterop.c
mono/metadata/gc-internals.h
mono/metadata/gc.c
mono/metadata/marshal.c
mono/metadata/object.c
mono/metadata/remoting.c
mono/metadata/threads-types.h
mono/metadata/threads.c

index 774151fa5b94ec1f68243e0de603818e873a8064..3155861850469b91e0902c7dc8c015dc59b091d9 100644 (file)
@@ -2439,12 +2439,8 @@ unload_thread_main (void *arg)
 
        /* Have to attach to the runtime so shutdown can wait for this thread */
        /* Force it to be attached to avoid racing during shutdown. */
-       thread = mono_thread_attach_full (mono_get_root_domain (), TRUE, &error);
-       if (!is_ok (&error)) {
-               data->failure_reason = g_strdup (mono_error_get_message (&error));
-               mono_error_cleanup (&error);
-               goto failure;
-       }
+       thread = mono_thread_attach_full (mono_get_root_domain (), TRUE);
+
        mono_thread_set_name_internal (thread->internal_thread, mono_string_new (mono_get_root_domain (), "Domain unloader"), TRUE, &error);
        if (!is_ok (&error)) {
                data->failure_reason = g_strdup (mono_error_get_message (&error));
index d500fb626ce2a66029d2687aa1ffbd3888ed6f88..889c19ece4f786221a020fc365cebe41983b7734 100644 (file)
@@ -1984,8 +1984,7 @@ cominterop_get_ccw_checked (MonoObject* object, MonoClass* itf, MonoError *error
                g_hash_table_insert (ccw_hash, GINT_TO_POINTER (mono_object_hash (object)), ccw_list);
                mono_cominterop_unlock ();
                /* register for finalization to clean up ccw */
-               mono_object_register_finalizer (object, error);
-               return_val_if_nok (error, NULL);
+               mono_object_register_finalizer (object);
        }
 
        cinfo = mono_custom_attrs_from_class_checked (itf, error);
index 52d950d8cca7b8469db99a3f95dcd751d6a24f4b..ff44969a81de215b72e081ea7567b8a3fdcda1f7 100644 (file)
@@ -66,7 +66,7 @@
 /* useful until we keep track of gc-references in corlib etc. */
 #define IS_GC_REFERENCE(class,t) (mono_gc_is_moving () ? FALSE : ((t)->type == MONO_TYPE_U && (class)->image == mono_defaults.corlib))
 
-void   mono_object_register_finalizer               (MonoObject  *obj, MonoError *error);
+void   mono_object_register_finalizer               (MonoObject  *obj);
 void   ves_icall_System_GC_InternalCollect          (int          generation);
 gint64 ves_icall_System_GC_GetTotalMemory           (MonoBoolean  forceCollection);
 void   ves_icall_System_GC_KeepAlive                (MonoObject  *obj);
index 45c98ea73d715492293b924676d1b31fbbefd137..79293d97b9910f8440b5d69e627551a27a031c2a 100644 (file)
@@ -73,7 +73,7 @@ static MonoCoopCond exited_cond;
 
 static MonoInternalThread *gc_thread;
 
-static void object_register_finalizer (MonoObject *obj, void (*callback)(void *, void*), MonoError *error);
+static void object_register_finalizer (MonoObject *obj, void (*callback)(void *, void*));
 
 static void reference_queue_proccess_all (void);
 static void mono_reference_queue_cleanup (void);
@@ -166,8 +166,7 @@ mono_gc_run_finalize (void *obj, void *data)
 #endif
 
        /* make sure the finalizer is not called again if the object is resurrected */
-       object_register_finalizer ((MonoObject *)obj, NULL, &error);
-       mono_error_assert_ok (&error); /* FIXME don't swallow the error */
+       object_register_finalizer ((MonoObject *)obj, NULL);
 
        if (log_finalizers)
                g_log ("mono-gc-finalizers", G_LOG_LEVEL_MESSAGE, "<%s at %p> Registered finalizer as processed.", o->vtable->klass->name, o);
@@ -280,14 +279,12 @@ unhandled_error:
 void
 mono_gc_finalize_threadpool_threads (void)
 {
-       MonoError error;
        while (threads_to_finalize) {
                MonoInternalThread *thread = (MonoInternalThread*) mono_mlist_get_data (threads_to_finalize);
 
                /* Force finalization of the thread. */
                thread->threadpool_thread = FALSE;
-               mono_object_register_finalizer ((MonoObject*)thread, &error);
-               mono_error_assert_ok (&error); /* FIXME don't swallow the error */
+               mono_object_register_finalizer ((MonoObject*)thread);
 
                mono_gc_run_finalize (thread, NULL);
 
@@ -317,16 +314,11 @@ mono_gc_out_of_memory (size_t size)
  * since that, too, can cause the underlying pointer to be offset.
  */
 static void
-object_register_finalizer (MonoObject *obj, void (*callback)(void *, void*), MonoError *error)
+object_register_finalizer (MonoObject *obj, void (*callback)(void *, void*))
 {
        MonoDomain *domain;
 
-       mono_error_init (error);
-
-       if (obj == NULL) {
-               mono_error_set_argument_null (error, "obj", "");
-               return;
-       }
+       g_assert (obj != NULL);
 
        domain = obj->vtable->domain;
 
@@ -368,10 +360,10 @@ object_register_finalizer (MonoObject *obj, void (*callback)(void *, void*), Mon
  * 
  */
 void
-mono_object_register_finalizer (MonoObject *obj, MonoError *error)
+mono_object_register_finalizer (MonoObject *obj)
 {
        /* g_print ("Registered finalizer on %p %s.%s\n", obj, mono_object_class (obj)->name_space, mono_object_class (obj)->name); */
-       object_register_finalizer (obj, mono_gc_run_finalize, error);
+       object_register_finalizer (obj, mono_gc_run_finalize);
 }
 
 /**
@@ -491,19 +483,14 @@ ves_icall_System_GC_KeepAlive (MonoObject *obj)
 void
 ves_icall_System_GC_ReRegisterForFinalize (MonoObject *obj)
 {
-       MonoError error;
-
        MONO_CHECK_ARG_NULL (obj,);
 
-       object_register_finalizer (obj, mono_gc_run_finalize, &error);
-       mono_error_set_pending_exception (&error);
+       object_register_finalizer (obj, mono_gc_run_finalize);
 }
 
 void
 ves_icall_System_GC_SuppressFinalize (MonoObject *obj)
 {
-       MonoError error;
-
        MONO_CHECK_ARG_NULL (obj,);
 
        /* delegates have no finalizers, but we register them to deal with the
@@ -517,8 +504,7 @@ ves_icall_System_GC_SuppressFinalize (MonoObject *obj)
         * generated for it that needs cleaned up, but user wants to suppress
         * their derived object finalizer. */
 
-       object_register_finalizer (obj, NULL, &error);
-       mono_error_set_pending_exception (&error);
+       object_register_finalizer (obj, NULL);
 }
 
 void
@@ -1127,18 +1113,18 @@ mono_gc_reference_queue_new (mono_reference_queue_callback callback)
 gboolean
 mono_gc_reference_queue_add (MonoReferenceQueue *queue, MonoObject *obj, void *user_data)
 {
-       MonoError error;
        RefQueueEntry *entry;
        if (queue->should_be_deleted)
                return FALSE;
 
+       g_assert (obj != NULL);
+
        entry = g_new0 (RefQueueEntry, 1);
        entry->user_data = user_data;
        entry->domain = mono_object_domain (obj);
 
        entry->gchandle = mono_gchandle_new_weakref (obj, TRUE);
-       mono_object_register_finalizer (obj, &error);
-       mono_error_assert_ok (&error);
+       mono_object_register_finalizer (obj);
 
        ref_list_push (&queue->queue, entry);
        return TRUE;
index 4e734a7a5f93854d490d34f231098109a19aa0f7..80fe883c48f5f915f81aa25150cbb35daf8e6ca3 100644 (file)
@@ -407,14 +407,10 @@ mono_delegate_to_ftnptr (MonoDelegate *delegate)
        delegate_hash_table_add (delegate);
 
        /* when the object is collected, collect the dynamic method, too */
-       mono_object_register_finalizer ((MonoObject*)delegate, &error);
-       if (!is_ok (&error))
-               goto fail2;
+       mono_object_register_finalizer ((MonoObject*)delegate);
 
        return delegate->delegate_trampoline;
 
-fail2:
-       delegate_hash_table_remove (delegate);
 fail:
        mono_gchandle_free (target_handle);
        mono_error_set_pending_exception (&error);
index c5fa20e1bb1e83f5c4010016c514e5ba4de3fe46..eb7738bf45ac94a5ee17c4d852c41da9c5df8e8f 100644 (file)
@@ -5156,7 +5156,7 @@ mono_object_new_pinned (MonoDomain *domain, MonoClass *klass, MonoError *error)
        if (G_UNLIKELY (!o))
                mono_error_set_out_of_memory (error, "Could not allocate %i bytes", mono_class_instance_size (klass));
        else if (G_UNLIKELY (vtable->klass->has_finalize))
-               mono_object_register_finalizer (o, error);
+               mono_object_register_finalizer (o);
 
        return o;
 }
@@ -5286,7 +5286,7 @@ mono_object_new_alloc_specific_checked (MonoVTable *vtable, MonoError *error)
        if (G_UNLIKELY (!o))
                mono_error_set_out_of_memory (error, "Could not allocate %i bytes", vtable->klass->instance_size);
        else if (G_UNLIKELY (vtable->klass->has_finalize))
-               mono_object_register_finalizer (o, error);
+               mono_object_register_finalizer (o);
 
        return o;
 }
@@ -5374,7 +5374,7 @@ mono_object_new_mature (MonoVTable *vtable, MonoError *error)
        if (G_UNLIKELY (!o))
                mono_error_set_out_of_memory (error, "Could not allocate %i bytes", vtable->klass->instance_size);
        else if (G_UNLIKELY (vtable->klass->has_finalize))
-               mono_object_register_finalizer (o, error);
+               mono_object_register_finalizer (o);
 
        return o;
 }
@@ -5489,7 +5489,7 @@ mono_object_clone_checked (MonoObject *obj, MonoError *error)
        mono_gc_wbarrier_object_copy (o, obj);
 
        if (obj->vtable->klass->has_finalize)
-               mono_object_register_finalizer (o, error);
+               mono_object_register_finalizer (o);
        return o;
 }
 
@@ -6264,7 +6264,7 @@ mono_value_box_checked (MonoDomain *domain, MonoClass *klass, gpointer value, Mo
 #endif
 #endif
        if (klass->has_finalize) {
-               mono_object_register_finalizer (res, error);
+               mono_object_register_finalizer (res);
                return_val_if_nok (error, NULL);
        }
        return res;
index e36eed07e37a50b15e863bfdc8808af2cdb19f8c..6e09671dac420ab0e68a7a4b2a2e080f1cac01f6 100644 (file)
@@ -504,6 +504,8 @@ mono_marshal_get_remoting_invoke (MonoMethod *method)
 /* mono_marshal_xdomain_copy_out_value()
  * Copies the contents of the src instance into the dst instance. src and dst
  * must have the same type, and if they are arrays, the same size.
+ *
+ * This is an icall, it may use mono_error_set_pending_exception
  */
 static void
 mono_marshal_xdomain_copy_out_value (MonoObject *src, MonoObject *dst)
@@ -523,7 +525,8 @@ mono_marshal_xdomain_copy_out_value (MonoObject *src, MonoObject *dst)
                        for (i = 0; i < len; i++) {
                                MonoObject *item = (MonoObject *)mono_array_get ((MonoArray *)src, gpointer, i);
                                MonoObject *item_copy = mono_marshal_xdomain_copy_value (item, &error);
-                               mono_error_raise_exception (&error); /* FIXME don't raise here */
+                               if (mono_error_set_pending_exception (&error))
+                                       return;
                                mono_array_setref ((MonoArray *)dst, i, item_copy);
                        }
                } else {
@@ -1259,7 +1262,7 @@ mono_marshal_load_remoting_wrapper (MonoRealProxy *rp, MonoMethod *method)
        else
                marshal_method = mono_marshal_get_remoting_invoke (method);
        gpointer compiled_ptr = mono_compile_method_checked (marshal_method, &error);
-       mono_error_raise_exception (&error); /* FIXME don't raise here */
+       mono_error_assert_ok (&error);
        return compiled_ptr;
 }
 
index 57d76b425478e79953f9ce11feac035bc8939967..68f8ec7851c3e1a13fa19f43c8aae37e285c6454 100644 (file)
@@ -248,7 +248,7 @@ gboolean
 mono_thread_create_checked (MonoDomain *domain, gpointer func, gpointer arg, MonoError *error);
 
 MonoThread *
-mono_thread_attach_full (MonoDomain *domain, gboolean force_attach, MonoError *error);
+mono_thread_attach_full (MonoDomain *domain, gboolean force_attach);
 
 void mono_thread_init_tls (void);
 
index d00b01f812933a860cf15b16c87f58d57ca87714..50ac9158aae755b6e03de139dc42989f42aa5e38 100644 (file)
@@ -574,21 +574,22 @@ set_current_thread_for_domain (MonoDomain *domain, MonoInternalThread *thread, M
 }
 
 static MonoThread*
-create_thread_object (MonoDomain *domain, MonoError *error)
+create_thread_object (MonoDomain *domain)
 {
+       MonoError error;
        MonoVTable *vt = mono_class_vtable (domain, mono_defaults.thread_class);
-       MonoThread *t = (MonoThread*)mono_object_new_mature (vt, error);
+       MonoThread *t = (MonoThread*)mono_object_new_mature (vt, &error);
+       /* only possible failure mode is OOM, from which we don't expect to recover. */
+       mono_error_assert_ok (&error);
        return t;
 }
 
 static MonoThread*
-new_thread_with_internal (MonoDomain *domain, MonoInternalThread *internal, MonoError *error)
+new_thread_with_internal (MonoDomain *domain, MonoInternalThread *internal)
 {
        MonoThread *thread;
 
-       thread = create_thread_object (domain, error);
-       if (!mono_error_ok (error))
-               return NULL;
+       thread = create_thread_object (domain);
 
        MONO_OBJECT_SETREF (thread, internal_thread, internal);
 
@@ -596,15 +597,16 @@ new_thread_with_internal (MonoDomain *domain, MonoInternalThread *internal, Mono
 }
 
 static MonoInternalThread*
-create_internal_thread (MonoError *error)
+create_internal_thread ()
 {
+       MonoError error;
        MonoInternalThread *thread;
        MonoVTable *vt;
 
        vt = mono_class_vtable (mono_get_root_domain (), mono_defaults.internal_thread_class);
-       thread = (MonoInternalThread*) mono_object_new_mature (vt, error);
-       if (!mono_error_ok (error))
-               return NULL;
+       thread = (MonoInternalThread*) mono_object_new_mature (vt, &error);
+       /* only possible failure mode is OOM, from which we don't exect to recover */
+       mono_error_assert_ok (&error);
 
        thread->synch_cs = g_new0 (MonoCoopMutex, 1);
        mono_coop_mutex_init_recursive (thread->synch_cs);
@@ -620,14 +622,12 @@ create_internal_thread (MonoError *error)
 }
 
 static gboolean
-init_root_domain_thread (MonoInternalThread *thread, MonoThread *candidate, MonoError *error)
+init_root_domain_thread (MonoInternalThread *thread, MonoThread *candidate)
 {
        MonoDomain *domain = mono_get_root_domain ();
 
-       mono_error_init (error);
        if (!candidate || candidate->obj.vtable->domain != domain) {
-               candidate = new_thread_with_internal (domain, thread, error);
-               return_val_if_nok (error, FALSE);
+               candidate = new_thread_with_internal (domain, thread);
        }
        set_current_thread_for_domain (domain, thread, candidate);
        g_assert (!thread->root_domain_thread);
@@ -686,8 +686,7 @@ static guint32 WINAPI start_wrapper_internal(void *data)
        /* We have to do this here because mono_thread_new_init()
           requires that root_domain_thread is set up. */
        thread_adjust_static_data (internal);
-       init_root_domain_thread (internal, start_info->obj, &error);
-       mono_error_raise_exception (&error); /* FIXME don't raise here */
+       init_root_domain_thread (internal, start_info->obj);
 
        /* This MUST be called before any managed code can be
         * executed, as it calls the callback function that (for the
@@ -927,11 +926,9 @@ mono_thread_create_internal (MonoDomain *domain, gpointer func, gpointer arg, gb
 
        mono_error_init (error);
 
-       thread = create_thread_object (domain, error);
-       return_val_if_nok (error, NULL);
+       thread = create_thread_object (domain);
 
-       internal = create_internal_thread (error);
-       return_val_if_nok (error, NULL);
+       internal = create_internal_thread ();
 
        MONO_OBJECT_SETREF (thread, internal_thread, internal);
 
@@ -969,15 +966,13 @@ mono_thread_create_checked (MonoDomain *domain, gpointer func, gpointer arg, Mon
 MonoThread *
 mono_thread_attach (MonoDomain *domain)
 {
-       MonoError error;
-       MonoThread *thread = mono_thread_attach_full (domain, FALSE, &error);
-       mono_error_raise_exception (&error);
+       MonoThread *thread = mono_thread_attach_full (domain, FALSE);
 
        return thread;
 }
 
 MonoThread *
-mono_thread_attach_full (MonoDomain *domain, gboolean force_attach, MonoError *error)
+mono_thread_attach_full (MonoDomain *domain, gboolean force_attach)
 {
        MonoThreadInfo *info;
        MonoInternalThread *thread;
@@ -985,8 +980,6 @@ mono_thread_attach_full (MonoDomain *domain, gboolean force_attach, MonoError *e
        HANDLE thread_handle;
        MonoNativeThreadId tid;
 
-       mono_error_init (error);
-
        if ((thread = mono_thread_internal_current ())) {
                if (domain != mono_domain_get ())
                        mono_domain_set (domain, TRUE);
@@ -998,9 +991,7 @@ mono_thread_attach_full (MonoDomain *domain, gboolean force_attach, MonoError *e
                g_error ("Thread %"G_GSIZE_FORMAT" calling into managed code is not registered with the GC. On UNIX, this can be fixed by #include-ing <gc.h> before <pthread.h> in the file containing the thread creation code.", mono_native_thread_id_get ());
        }
 
-       thread = create_internal_thread (error);
-       if (!mono_error_ok (error))
-               return NULL;
+       thread = create_internal_thread ();
 
        thread_handle = mono_thread_info_open_handle ();
        g_assert (thread_handle);
@@ -1018,9 +1009,7 @@ mono_thread_attach_full (MonoDomain *domain, gboolean force_attach, MonoError *e
        thread->thread_info = info;
        thread->small_id = info->small_id;
 
-       current_thread = new_thread_with_internal (domain, thread, error);
-       if (!mono_error_ok (error))
-               return NULL;
+       current_thread = new_thread_with_internal (domain, thread);
 
        if (!handle_store (current_thread, force_attach)) {
                /* Mono is shutting down, so just wait for the end */
@@ -1035,8 +1024,7 @@ mono_thread_attach_full (MonoDomain *domain, gboolean force_attach, MonoError *e
 
        thread_adjust_static_data (thread);
 
-       init_root_domain_thread (thread, current_thread, error);
-       return_val_if_nok (error, NULL);
+       init_root_domain_thread (thread, current_thread);
 
        if (domain != mono_get_root_domain ())
                set_current_thread_for_domain (domain, thread, current_thread);
@@ -1128,12 +1116,9 @@ mono_thread_exit ()
 void
 ves_icall_System_Threading_Thread_ConstructInternalThread (MonoThread *this_obj)
 {
-       MonoError error;
        MonoInternalThread *internal;
 
-       internal = create_internal_thread (&error);
-       if (mono_error_set_pending_exception (&error))
-               return;
+       internal = create_internal_thread ();
 
        internal->state = ThreadState_Unstarted;
 
@@ -1492,7 +1477,6 @@ ves_icall_System_Threading_Thread_ByteArrayToCurrentDomain (MonoArray *arr)
 MonoThread *
 mono_thread_current (void)
 {
-       MonoError error;
        MonoDomain *domain = mono_domain_get ();
        MonoInternalThread *internal = mono_thread_internal_current ();
        MonoThread **current_thread_ptr;
@@ -1502,8 +1486,7 @@ mono_thread_current (void)
 
        if (!*current_thread_ptr) {
                g_assert (domain != mono_get_root_domain ());
-               *current_thread_ptr = new_thread_with_internal (domain, internal, &error);
-               mono_error_raise_exception (&error); /* FIXME don't raise here */
+               *current_thread_ptr = new_thread_with_internal (domain, internal);
        }
        return *current_thread_ptr;
 }
@@ -1512,7 +1495,6 @@ mono_thread_current (void)
 static MonoThread *
 mono_thread_current_for_thread (MonoInternalThread *internal)
 {
-       MonoError error;
        MonoDomain *domain = mono_domain_get ();
        MonoThread **current_thread_ptr;
 
@@ -1521,8 +1503,7 @@ mono_thread_current_for_thread (MonoInternalThread *internal)
 
        if (!*current_thread_ptr) {
                g_assert (domain != mono_get_root_domain ());
-               *current_thread_ptr = new_thread_with_internal (domain, internal, &error);
-               mono_error_raise_exception (&error); /* FIXME don't raise here */
+               *current_thread_ptr = new_thread_with_internal (domain, internal);
        }
        return *current_thread_ptr;
 }
@@ -5114,7 +5095,6 @@ ves_icall_System_Threading_Thread_GetStackTraces (MonoArray **out_threads, MonoA
 gpointer
 mono_threads_attach_coop (MonoDomain *domain, gpointer *dummy)
 {
-       MonoError error;
        MonoDomain *orig;
        gboolean fresh_thread;
 
@@ -5135,8 +5115,7 @@ mono_threads_attach_coop (MonoDomain *domain, gpointer *dummy)
        }
 
        if (!mono_thread_internal_current ()) {
-               mono_thread_attach_full (domain, FALSE, &error);
-               mono_error_assert_ok (&error);
+               mono_thread_attach_full (domain, FALSE);
 
                // #678164
                mono_thread_set_state (mono_thread_internal_current (), ThreadState_Background);