File 0545-erts-Robustify-yielding-ets-insert-2-and-e... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 0545-erts-Robustify-yielding-ets-insert-2-and-ets-insert_.patch of Package erlang (Revision 3e53aa07e56d6cc0c5ffbbfd2f14b043)

Currently displaying revision 3e53aa07e56d6cc0c5ffbbfd2f14b043 , Show latest

From 9b5f5a67557ebd0583a684fe544a42e0d081df43 Mon Sep 17 00:00:00 2001
From: Sverker Eriksson <sverker@erlang.org>
Date: Wed, 24 Aug 2022 12:35:48 +0200
Subject: [PATCH 5/5] erts: Robustify yielding ets:insert/2 and
 ets:insert_new/2

Symptom:
    VM crash if table is deleted or renamed during ets:insert
    and ets:insert_new with long list of tuples.

* Handle table deletion during yielding insert correctly.
  Both deletion by ets:delete/1 and owner process exit.
  We no longer commit the entire list for insert. If
  table is deleted, the remaining list with uninserted
  tuples will be freed.

* Handle table renaming correctly. Do not change table midstream.
  Only use initial BIF_ARG_1 for first lookup. Then use 'btid'
  to identify the table.
  If table is renamed before we go DB_BUSY (and force others to help)
  the insert will fail. Otherwise it could be observed that
  the insert op happened after the table was renamed.

* Make sure a successful assisted insert op returns 'true' even if table was
  deleted by someone else before the initial caller returns.

* Make sure BIF arguments are restored if ets:insert fails after trapping.
---
 erts/emulator/beam/erl_db.c      | 455 ++++++++++++++++++-------------
 erts/emulator/beam/erl_db_util.h |   6 +-
 lib/stdlib/test/ets_SUITE.erl    | 193 ++++++++++++-
 3 files changed, 453 insertions(+), 201 deletions(-)

diff --git a/erts/emulator/beam/erl_db.c b/erts/emulator/beam/erl_db.c
index a32c9803e7..e953381567 100644
--- a/erts/emulator/beam/erl_db.c
+++ b/erts/emulator/beam/erl_db.c
@@ -241,9 +241,16 @@ static void table_dec_refc(DbTable *tb, erts_aint_t min_val)
 	schedule_free_dbtable(tb);
 }
 
+static ERTS_INLINE DbTable* btid2tab(Binary* btid)
+{
+    erts_atomic_t *tbref = erts_binary_to_magic_indirection(btid);
+    return (DbTable *) erts_atomic_read_nob(tbref);
+}
+
 static int
-db_table_tid_destructor(Binary *unused)
+db_table_tid_destructor(Binary *btid)
 {
+    ASSERT(btid2tab(btid) == NULL);
     return 1;
 }
 
@@ -262,12 +269,6 @@ make_btid(DbTable *tb)
     erts_refc_inc(&btid->intern.refc, 1);
 }
 
-static ERTS_INLINE DbTable* btid2tab(Binary* btid)
-{
-    erts_atomic_t *tbref = erts_binary_to_magic_indirection(btid);
-    return (DbTable *) erts_atomic_read_nob(tbref);
-}
-
 static DbTable *
 tid2tab(Eterm tid, Eterm *error_info_p)
 {
@@ -425,6 +426,8 @@ static BIF_RETTYPE ets_select1(Process* p, int bif_ix, Eterm arg1);
 static BIF_RETTYPE ets_select2(Process* p, DbTable*, Eterm tid, Eterm ms);
 static BIF_RETTYPE ets_select3(Process* p, DbTable*, Eterm tid, Eterm ms, Sint chunk_size);
 
+static BIF_RETTYPE ets_insert_2_list_continuation(Process* p,
+                                                  struct ets_insert_2_list_info* ctx);
 
 /* 
  * Exported global
@@ -457,8 +460,8 @@ free_dbtable(void *vtb)
         erts_mtx_destroy(&tb->common.fixlock);
     }
 
-    if (tb->common.btid)
-        erts_bin_release(tb->common.btid);
+    ASSERT(tb->common.btid);
+    erts_bin_release(tb->common.btid);
 
     erts_flxctr_destroy(&tb->common.counters, ERTS_ALC_T_ETS_CTRS);
     erts_free(ERTS_ALC_T_DB_TABLE, tb);
@@ -466,10 +469,7 @@ free_dbtable(void *vtb)
 
 static void schedule_free_dbtable(DbTable* tb)
 {
-    /*
-     * NON-SMP case: Caller is *not* allowed to access the *tb
-     *               structure after this function has returned!          
-     * SMP case:     Caller is allowed to access the *common* part of the *tb
+    /* SMP case:     Caller is allowed to access the *common* part of the *tb
      *  	     structure until the bif has returned (we typically need to
      *  	     unlock the table lock after this function has returned).
      *  	     Caller is *not* allowed to access the specialized part
@@ -721,24 +721,12 @@ static DbTable* handle_lacking_permission(Process* p, DbTable* tb,
                                           Uint* freason_p)
 {
     if (tb->common.status & DB_BUSY) {
-        void* continuation_state;
         if (!db_is_exclusive(tb, kind)) {
             db_unlock(tb, kind);
             db_lock(tb, LCK_WRITE);
         }
-        continuation_state = (void*)erts_atomic_read_nob(&tb->common.continuation_state);
-        if (continuation_state != NULL) {
-            const long iterations_per_red = 10;
-            const long reds = iterations_per_red * ERTS_BIF_REDS_LEFT(p);
-            long nr_of_reductions = DBG_RANDOM_REDS(reds, (Uint)freason_p);
-            const long init_reds = nr_of_reductions;
-            tb->common.continuation(&nr_of_reductions,
-                                    &continuation_state,
-                                    NULL);
-            if (continuation_state == NULL) {
-                erts_atomic_set_relb(&tb->common.continuation_state, (Sint)NULL);
-            }
-            BUMP_REDS(p, (init_reds - nr_of_reductions) / iterations_per_red);
+        if (tb->common.continuation_ctx) {
+            ets_insert_2_list_continuation(p, tb->common.continuation_ctx);
         } else {
             delete_all_objects_continue(p, tb);
         }
@@ -868,15 +856,31 @@ DbTable* db_get_table(Process *p,
     return db_get_table_aux(p, id, what, kind, 0, freason_p);
 }
 
-static BIF_RETTYPE db_get_table_or_fail_return(DbTable **tb, /* out */
-                                               Eterm table_id,
-                                               Uint32 what,
-                                               db_lock_kind_t kind,
-                                               Uint bif_ix,
-                                               Process* p)
+static DbTable* db_get_table_or_fail_return(Binary* btid,
+                                            Uint32 what,
+                                            db_lock_kind_t kind,
+                                            Uint bif_ix,
+                                            Process* p)
 {
-    DB_GET_TABLE(*tb, table_id, what, kind, bif_ix, NULL, p);
-    return THE_NON_VALUE;
+    DbTable* tb = btid2tab(btid);
+    if (!tb) {
+        p->freason = BADARG | EXF_HAS_EXT_INFO;
+        p->fvalue = EXI_ID;
+    }
+    else {
+        /* The lock has to be taken to complete the operation */
+        db_lock(tb, LCK_WRITE);
+        if (!(tb->common.status & what)) {
+            Uint freason;
+            tb = handle_lacking_permission(p, tb, kind, &freason);
+            if (!tb) {
+                BIF_RETTYPE ret = db_bif_fail(p, freason, bif_ix, NULL);
+                ASSERT(ret == THE_NON_VALUE); (void)ret;
+            }
+        }
+    }
+
+    return tb;
 }
 
 static int insert_named_tab(Eterm name_atom, DbTable* tb, int have_lock)
@@ -1508,20 +1512,38 @@ BIF_RETTYPE ets_update_counter_4(BIF_ALIST_4)
 }
 
 typedef enum {
+    /*
+     * Prepare phase. Done only by the process calling ets:insert/insert_new.
+     * All tuples to insert are allocated and copied without table lock.
+     */
     ETS_INSERT_2_LIST_PROCESS_LOCAL,
-    ETS_INSERT_2_LIST_FAILED_TO_GET_LOCK,
-    ETS_INSERT_2_LIST_FAILED_TO_GET_LOCK_DESTROY,
+
+    /*
+     * Commit phase. May be assisted by other calling processes.
+     * Prepared tuples inserted with table lock.
+     */
     ETS_INSERT_2_LIST_GLOBAL
 } ets_insert_2_list_status;
 
-typedef struct {
+typedef struct ets_insert_2_list_info {
     ets_insert_2_list_status status;
-    BIF_RETTYPE destroy_return_value;
-    DbTable* tb;
+    Binary* btid; /* identifies the table between traps */
+    Eterm tid;    /* arg1, also used to detect table name change */
+    Eterm list;   /* arg2 */
+    DbTable* tb;  /* cached tb, does not keep table alive between traps */
     void* continuation_state;
-    Binary* continuation_res_bin;
+    erts_atomic_t return_value; /* Eterm: 'true', 'false' or THE_NON_VALUE */
 } ets_insert_2_list_info;
 
+static void cancel_trap_continuation(DbTable* tb)
+{
+    ets_insert_2_list_info* ctx = tb->common.continuation_ctx;
+
+    if (ctx) {
+        tb->common.continuation_ctx = NULL;
+        erts_bin_release(&(ERTS_MAGIC_BIN_FROM_DATA(ctx)->binary));
+    }
+}
 
 static ERTS_INLINE BIF_RETTYPE
 ets_cret_to_return_value(Process* p, int cret)
@@ -1685,63 +1707,77 @@ static int ets_insert_new_2_dbterm_list_has_member(DbTable* tb, void* db_term_li
 }
 
 static void ets_insert_2_list_insert_db_term_list(DbTable* tb,
-                                                  void* list)
+                                                 void* db_term_list)
 {
-    void* lst = list;
+    void* tail = db_term_list;
     void* term = NULL;
     DbTableMethod* meth = tb->common.meth;
+    int compress = tb->common.compress;
     do {
         LOCAL_VARIABLE(SWord, consumed_reds);
         consumed_reds = 1;
-        term = meth->db_dbterm_list_remove_first(&lst);
+        term = meth->db_dbterm_list_remove_first(&tail);
         meth->db_put_dbterm(tb, term, 0, &consumed_reds);
         YCF_CONSUME_REDS(consumed_reds);
-    } while (lst != NULL);
+    } while (tail != NULL);
     return;
+
+    YCF_SPECIAL_CODE_START(ON_DESTROY_STATE); {
+        ets_insert_2_list_destroy_copied_dbterms(meth,
+                                                 compress,
+                                                 tail);
+    } YCF_SPECIAL_CODE_END();
+
 }
 
-static void ets_insert_2_list_lock_tbl(Eterm table_id,
-                                       Process* p,
-                                       Uint bif_ix,
-                                       ets_insert_2_list_status on_success_status)
+static int ets_insert_2_list_lock_tbl(Binary *btid,
+                                      Process* p,
+                                      Uint bif_ix,
+                                      ets_insert_2_list_status on_success_status)
 {
-    BIF_RETTYPE fail_ret;
     DbTable* tb;
+
     do {
-        fail_ret = db_get_table_or_fail_return(&tb,
-                                               table_id,
-                                               DB_WRITE,
-                                               LCK_WRITE,
-                                               bif_ix,
-                                               p);
+        LOCAL_VARIABLE(ets_insert_2_list_info*,ctx);
+        ctx = YCF_GET_EXTRA_CONTEXT();
+        ASSERT(ctx->status != ETS_INSERT_2_LIST_GLOBAL);
+
+        tb = db_get_table_or_fail_return(btid,
+                                         DB_WRITE,
+                                         LCK_WRITE,
+                                         bif_ix,
+                                         p);
+        ASSERT(ctx->status != ETS_INSERT_2_LIST_GLOBAL);
+
         if (tb == NULL) {
-            ets_insert_2_list_info *ctx = YCF_GET_EXTRA_CONTEXT();
             if (p->freason == TRAP) {
-                ctx->status = ETS_INSERT_2_LIST_FAILED_TO_GET_LOCK;
+                YCF_YIELD();
             } else {
-                ctx->status = ETS_INSERT_2_LIST_FAILED_TO_GET_LOCK_DESTROY;
-                ctx->destroy_return_value = fail_ret;
+                return 0;
             }
-#ifdef DEBUG
+        }
+        else if (is_atom(ctx->tid) && tb->common.the_name != ctx->tid) {
             /*
-             *  Setting ctx to NULL to avoid that YCF crashes with a
-             *  pointer to stack error when running a debug
-             *  build. YCF_GET_EXTRA_CONTEXT() may change between
-             *  yields as we use stack allocated data for the context
-             *  before the first yield so it is important that the
-             *  context is obtained again with YCF_GET_EXTRA_CONTEXT()
-             *  if a yield might have happened.
+             * The table has been renamed. We have to fail. This named
+             * insert op might otherwise be observable as if it happened
+             * AFTER the table was renamed.
+             *
+             * Note that we are allowed to fail here as long as there is no
+             * atomic name change op from one table to another.
              */
-            ctx = NULL;
-#endif
-            YCF_YIELD();
-        } else {
-            ets_insert_2_list_info *ctx = YCF_GET_EXTRA_CONTEXT();
+            p->freason = BADARG | EXF_HAS_EXT_INFO;
+            p->fvalue = EXI_ID;
+            db_unlock(tb, LCK_WRITE);
+            return 0;
+        }
+        else {
             ctx->status = on_success_status;
-            ASSERT(DB_LOCK_FREE(tb) || erts_lc_rwmtx_is_rwlocked(&tb->common.rwlock));
-            ASSERT(!(tb->common.status & DB_DELETE));
         }
     } while (tb == NULL);
+
+    ERTS_LC_ASSERT(DB_LOCK_FREE(tb) || erts_lc_rwmtx_is_rwlocked(&tb->common.rwlock));
+    ASSERT(!(tb->common.status & (DB_DELETE|DB_BUSY)));
+    return 1;
 }
 #endif /* YCF_FUNCTIONS */
 
@@ -1762,13 +1798,14 @@ static ERTS_INLINE int can_insert_without_yield(Uint32 tb_type,
 #ifdef YCF_FUNCTIONS
 static BIF_RETTYPE ets_insert_2_list(Process* p,
                                      Eterm table_id,
+                                     Binary* btid,
                                      DbTable *tb,
                                      Eterm list,
                                      int is_insert_new)
 {
     int cret = DB_ERROR_NONE;
-    void* db_term_list = NULL; /* OBS: memory managements depends on that
-                                  db_term_list is initialized to NULL */
+    void* db_term_list = NULL;
+    void* destroy_list = NULL;
     DbTableMethod* meth = tb->common.meth;
     int compressed = tb->common.compress;
     int keypos = tb->common.keypos;
@@ -1786,16 +1823,18 @@ static BIF_RETTYPE ets_insert_2_list(Process* p,
          * table. This is necessary to ensure that the correct reason
          * for the failure will be available in stack backtrace.
          */
-        ets_insert_2_list_lock_tbl(table_id, p, bif_ix, ETS_INSERT_2_LIST_PROCESS_LOCAL);
-        db_unlock(tb, LCK_WRITE);
-        ERTS_BIF_PREP_ERROR_TRAPPED2(ret, p, BADARG, BIF_TRAP_EXPORT(bif_ix), table_id, list);
-        return ret;
+        if (ets_insert_2_list_lock_tbl(btid, p, bif_ix, ETS_INSERT_2_LIST_PROCESS_LOCAL)) {
+            db_unlock(tb, LCK_WRITE);
+            p->freason = BADARG;
+        }
+        return THE_NON_VALUE;
     }
     if (can_insert_without_yield(tb_type, list_len, YCF_NR_OF_REDS_LEFT())) {
         long reds_boost;
         /* There is enough reductions left to do the inserts directly
            from the heap without yielding */
-        ets_insert_2_list_lock_tbl(table_id, p, bif_ix, ETS_INSERT_2_LIST_PROCESS_LOCAL);
+        if (!ets_insert_2_list_lock_tbl(btid, p, bif_ix, ETS_INSERT_2_LIST_PROCESS_LOCAL))
+            return THE_NON_VALUE;
         /* Ensure that we will not yield while inserting from heap */
         reds_boost = YCF_MAX_NR_OF_REDS - YCF_NR_OF_REDS_LEFT();
         YCF_SET_NR_OF_REDS_LEFT(YCF_MAX_NR_OF_REDS);
@@ -1815,44 +1854,46 @@ static BIF_RETTYPE ets_insert_2_list(Process* p,
     /* Copy term list from heap so that other processes can help */
     db_term_list =
         ets_insert_2_list_copy_term_list(meth, compressed, keypos, list);
+    destroy_list = db_term_list;
     /* Lock table */
-    ets_insert_2_list_lock_tbl(table_id, p, bif_ix, ETS_INSERT_2_LIST_GLOBAL);
-    /* The operation must complete after this point */
+    if (!ets_insert_2_list_lock_tbl(btid, p, bif_ix, ETS_INSERT_2_LIST_GLOBAL)) {
+        const Eterm fvalue = p->fvalue;
+        ASSERT(p->freason == (BADARG | EXF_HAS_EXT_INFO));
+
+        destroy_list = NULL;
+        ets_insert_2_list_destroy_copied_dbterms(meth,
+                                                 compressed,
+                                                 db_term_list);
+        /* Restore failure reason as we may have trapped during destroy */
+        p->freason = BADARG | EXF_HAS_EXT_INFO;
+        p->fvalue = fvalue;
+        return THE_NON_VALUE;
+    }
+
     if (is_insert_new) {
         if (ets_insert_new_2_dbterm_list_has_member(tb, db_term_list)) {
+            destroy_list = NULL;
             ets_insert_2_list_destroy_copied_dbterms(meth,
                                                      compressed,
                                                      db_term_list);
             cret = DB_ERROR_NONE_FALSE;
-        } else {
+        }
+        else {
+            destroy_list = NULL;
             ets_insert_2_list_insert_db_term_list(tb, db_term_list);
         }
-    } else {
-        ets_insert_2_list_insert_db_term_list(tb, db_term_list);
     }
-    if (tb->common.continuation != NULL) {
-        /* Uninstall the continuation from the table struct */
-        tb->common.continuation = NULL;
-        if (is_insert_new) {
-            int* result_ptr =
-                ERTS_MAGIC_BIN_DATA(tb->common.continuation_res_bin);
-            *result_ptr = cret;
-            erts_bin_release(tb->common.continuation_res_bin);
-        }
-        tb->common.status |= tb->common.type & (DB_PRIVATE|DB_PROTECTED|DB_PUBLIC);
-        tb->common.status &= ~DB_BUSY;
-        erts_atomic_set_relb(&tb->common.continuation_state, (Sint)NULL);
+    else {
+        destroy_list = NULL;
+        ets_insert_2_list_insert_db_term_list(tb, db_term_list);
     }
     
     return ets_cret_to_return_value(NULL, cret);
 
-    /* The following code will be executed if the initiating process
-       is killed before an ets_insert_2_list_lock_tbl call has
-       succeeded */
     YCF_SPECIAL_CODE_START(ON_DESTROY_STATE); {
         ets_insert_2_list_destroy_copied_dbterms(meth,
                                                  compressed,
-                                                 db_term_list);
+                                                 destroy_list);
     } YCF_SPECIAL_CODE_END();
 }
 #endif /* YCF_FUNCTIONS */
@@ -1887,48 +1928,76 @@ static void ets_insert_2_yield_free(void* data, void* ctx)
 static int ets_insert_2_list_yield_dtor(Binary* bin)
 {
     ets_insert_2_list_info* ctx = ERTS_MAGIC_BIN_DATA(bin);
-    if (ctx->status != ETS_INSERT_2_LIST_GLOBAL &&
-        ctx->continuation_state != NULL) {
-        /* The operation has not been committed to the table and has
-           not completed*/
+    if (ctx->continuation_state) {
         ets_insert_2_list_ycf_gen_destroy(ctx->continuation_state);
     }
+    erts_bin_release(ctx->btid);
     return 1;
 }
 
-static void ets_insert_2_list_continuation(long *reds_ptr,
-                                           void** state,
-                                           void* extra_context)
+#define ITERATIONS_PER_RED 8
+
+static BIF_RETTYPE
+ets_insert_2_list_continuation(Process* p,
+                               ets_insert_2_list_info* ctx)
 {
+    long reds = ITERATIONS_PER_RED * ERTS_BIF_REDS_LEFT(p);
+    long init_reds;
+    BIF_RETTYPE ret;
+
+    reds = DBG_RANDOM_REDS(reds, (Uint)ctx);
+    init_reds = reds;
+
+    ERTS_LC_ASSERT(ctx->status != ETS_INSERT_2_LIST_GLOBAL
+                   || DB_LOCK_FREE(tb)
+                   || erts_lc_rwmtx_is_rwlocked(&ctx->tb->common.rwlock));
+    ASSERT(ctx->continuation_state);
+
 #if defined(DEBUG) && defined(ARCH_64)
-    ycf_debug_set_stack_start(reds_ptr);
+    ycf_debug_set_stack_start(&reds);
 #endif
-    ets_insert_2_list_ycf_gen_continue(reds_ptr, state, extra_context);
+    ret = ets_insert_2_list_ycf_gen_continue(&reds,
+                                             &ctx->continuation_state,
+                                             ctx);
 #if defined(DEBUG) && defined(ARCH_64)
     ycf_debug_reset_stack_start();
 #endif
-}
 
-static int db_insert_new_2_res_bin_dtor(Binary *context_bin)
-{
-    (void)context_bin;
-    return 1;
+    if (ctx->continuation_state == NULL) {
+        if (is_value(ret)) {
+            ASSERT(ret == am_true || ret == am_false);
+            erts_atomic_set_nob(&ctx->return_value, ret);
+        }
+        if (ctx->status == ETS_INSERT_2_LIST_GLOBAL) {
+            DbTableCommon *tb = &ctx->tb->common;
+            if (tb->continuation_ctx) {
+                /* Uninstall the continuation from the table struct */
+                ASSERT(!(tb->status & DB_DELETE));
+                tb->status |= tb->type & (DB_PRIVATE|DB_PROTECTED|DB_PUBLIC);
+                tb->status &= ~DB_BUSY;
+                tb->continuation_ctx = NULL;
+                erts_bin_release(&(ERTS_MAGIC_BIN_FROM_DATA(ctx)->binary));
+            }
+        }
+    }
+    BUMP_REDS(p, (init_reds - reds) / ITERATIONS_PER_RED);
+    return ret;
 }
 
-#define ITERATIONS_PER_RED 8
-
 static BIF_RETTYPE ets_insert_2_list_driver(Process* p,
                                             Eterm tid,
                                             Eterm list,
                                             int is_insert_new) {
-    const long reds = ITERATIONS_PER_RED * ERTS_BIF_REDS_LEFT(p);
-    long nr_of_reductions = DBG_RANDOM_REDS(reds, (Uint)&p);
-    const long init_reds = nr_of_reductions;
+#if defined(DEBUG) && defined(ARCH_64)
+    int dbg_ycf_stack_start;
+#endif
     ets_insert_2_list_info* ctx = NULL;
-    ets_insert_2_list_info ictx;
     BIF_RETTYPE ret = THE_NON_VALUE;
     Eterm state_mref = list;
     Uint bix = (is_insert_new ? BIF_ets_insert_new_2 : BIF_ets_insert_2);
+    ets_insert_2_list_info ictx;
+    int do_trap;
+
     if (is_internal_magic_ref(state_mref)) {
         Binary* state_bin = erts_magic_ref2bin(state_mref);
         if (ERTS_MAGIC_BIN_DESTRUCTOR(state_bin) != ets_insert_2_list_yield_dtor) {
@@ -1938,59 +2007,60 @@ static BIF_RETTYPE ets_insert_2_list_driver(Process* p,
         erts_set_gc_state(p, 1);
         ctx = ERTS_MAGIC_BIN_DATA(state_bin);
         if (ctx->status == ETS_INSERT_2_LIST_GLOBAL) {
-            /* An operation that can be helped by other operations is
-               handled here */
-            Uint freason;
-            int cret = DB_ERROR_NONE;
-            DbTable* tb;
-            /* First check if another process has completed the
-               operation without acquiring the lock */
-            tb = db_get_table(p, tid, DB_READ_TBL_STRUCT, LCK_NOLOCK_ACCESS,
-                              &freason);
-            ASSERT(tb || freason != TRAP);
-            if (tb != NULL &&
-                (void*)erts_atomic_read_acqb(&tb->common.continuation_state) ==
-                ctx->continuation_state) {
-                /* The lock has to be taken to complete the operation */
-                if (NULL == (tb = db_get_table(p, tid, DB_WRITE, LCK_WRITE, &freason))) {
-                    if (freason == TRAP){
-                        erts_set_gc_state(p, 0);
-                        return db_bif_fail(p, freason, bix, NULL);
-                    }
-                }
-                /* Must be done since the db_get_table call did not trap */
-                if (tb != NULL) {
+            DbTable* tb = btid2tab(ctx->btid);
+            if (tb) {
+                db_lock(tb, LCK_WRITE);
+                if (ctx != tb->common.continuation_ctx) {
                     db_unlock(tb, LCK_WRITE);
+                    tb = NULL;
                 }
             }
-            if (is_insert_new) {
-                int* res = ERTS_MAGIC_BIN_DATA(ctx->continuation_res_bin);
-                cret = *res;
+            if (!tb) {
+                /*
+                 * Operation completed/aborted by someone else.
+                 * Note: If insert was successful but table has been deleted,
+                 * we still return success. It would be wrong to fail the insert
+                 * if someone have seen the result (before the table was deleted).
+                 */
+                ret = erts_atomic_read_nob(&ctx->return_value);
+                if (is_value(ret)) {
+                    ASSERT(ret == am_true ||
+                           (ret == am_false && is_insert_new));
+                    return ret;
+                }
+                else {
+                    ASSERT(!tb || tb->common.status & DB_DELETE);
+                    BIF_ERROR(p, BADARG | EXF_HAS_EXT_INFO);
+                }
             }
-            return ets_cret_to_return_value(NULL, cret);
-        } else {
-#if defined(DEBUG) && defined(ARCH_64)
-            ycf_debug_set_stack_start(&nr_of_reductions);
-#endif
-            ret = ets_insert_2_list_ycf_gen_continue(&nr_of_reductions,
-                                                     &ctx->continuation_state,
-                                                     ctx);
-#if defined(DEBUG) && defined(ARCH_64)
-            ycf_debug_reset_stack_start();
-#endif
+            ASSERT((tb->common.status & (DB_WRITE|DB_BUSY))
+                   == DB_BUSY);
         }
+        ret = ets_insert_2_list_continuation(p, ctx);
+        ASSERT(ctx->continuation_state
+               || ret == am_true || ret == am_false || ret == THE_NON_VALUE);
     } else {
         /* Start call */
+        long reds = ITERATIONS_PER_RED * ERTS_BIF_REDS_LEFT(p);
+        long init_reds;
+
+        reds = DBG_RANDOM_REDS(reds, (Uint)p);
+        init_reds = reds;
+
         ictx.continuation_state = NULL;
         ictx.status = ETS_INSERT_2_LIST_PROCESS_LOCAL;
+        erts_atomic_init_nob(&ictx.return_value, THE_NON_VALUE);
         ictx.tb = NULL;
-        ctx = &ictx;
-        DB_GET_TABLE(ctx->tb, tid, DB_READ_TBL_STRUCT, LCK_NOLOCK_ACCESS, bix,
+        ictx.tid = tid;
+        ictx.list = list;
+        DB_GET_TABLE(ictx.tb, tid, DB_READ_TBL_STRUCT, LCK_NOLOCK_ACCESS, bix,
                      NULL, p);
+        ictx.btid = ictx.tb->common.btid;
+        ctx = &ictx;
 #if defined(DEBUG) && defined(ARCH_64)
-        ycf_debug_set_stack_start(&nr_of_reductions);
+        ycf_debug_set_stack_start(&dbg_ycf_stack_start);
 #endif
-        ret = ets_insert_2_list_ycf_gen_yielding(&nr_of_reductions,
+        ret = ets_insert_2_list_ycf_gen_yielding(&reds,
                                                  &ctx->continuation_state,
                                                  ctx,
                                                  ets_insert_2_yield_alloc,
@@ -2000,12 +2070,16 @@ static BIF_RETTYPE ets_insert_2_list_driver(Process* p,
                                                  NULL,
                                                  p,
                                                  tid,
+                                                 ctx->btid,
                                                  ctx->tb,
                                                  list,
                                                  is_insert_new);
 #if defined(DEBUG) && defined(ARCH_64)
         ycf_debug_reset_stack_start();
 #endif
+        ASSERT(ctx->continuation_state
+               || ret == am_true || ret == am_false || ret == THE_NON_VALUE);
+
         if (ctx->continuation_state != NULL) {
             Binary* state_bin = erts_create_magic_binary(sizeof(ets_insert_2_list_info),
                                                          ets_insert_2_list_yield_dtor);
@@ -2013,40 +2087,37 @@ static BIF_RETTYPE ets_insert_2_list_driver(Process* p,
             state_mref = erts_mk_magic_ref(&hp, &MSO(p), state_bin);
             ctx = ERTS_MAGIC_BIN_DATA(state_bin);
             *ctx = ictx;
+            erts_refc_inc(&ctx->btid->intern.refc, 2);
         }
-    }
-    BUMP_REDS(p, (init_reds - nr_of_reductions) / ITERATIONS_PER_RED);
-    if (ctx->status == ETS_INSERT_2_LIST_GLOBAL &&
-        ctx->continuation_state != NULL &&
-        ctx->tb->common.continuation == NULL) {
-        /* Install the continuation in the table structure so other
-           threads can help */
-        if (is_insert_new) {
-            Binary* bin =
-                erts_create_magic_binary(sizeof(int),
-                                         db_insert_new_2_res_bin_dtor);
-            Eterm* hp = HAlloc(p, ERTS_MAGIC_REF_THING_SIZE);
-            erts_mk_magic_ref(&hp, &MSO(p), bin);
-            erts_refc_inctest(&bin->intern.refc, 2);
-            ctx->tb->common.continuation_res_bin = bin;
-            ctx->continuation_res_bin = bin;
+        BUMP_REDS(p, (init_reds - reds) / ITERATIONS_PER_RED);
+    }
+    do_trap = (ctx->continuation_state != NULL);
+    if (do_trap) {
+        if (ctx->status == ETS_INSERT_2_LIST_GLOBAL &&
+            !ctx->tb->common.continuation_ctx) {
+            /* Install the continuation in the table structure so other
+               threads can help */
+            ctx->tb->common.status &= ~(DB_PRIVATE|DB_PROTECTED|DB_PUBLIC);
+            ctx->tb->common.status |= DB_BUSY;
+            ASSERT(ctx != &ictx);
+            erts_refc_inc(&(ERTS_MAGIC_BIN_FROM_DATA(ctx)->binary.intern.refc), 2);
+            ctx->tb->common.continuation_ctx = ctx;
         }
-        ctx->tb->common.continuation = ets_insert_2_list_continuation;
-        ctx->tb->common.status &= ~(DB_PRIVATE|DB_PROTECTED|DB_PUBLIC);
-        ctx->tb->common.status |= DB_BUSY;
-        erts_atomic_set_relb(&ctx->tb->common.continuation_state,
-                             (Sint)ctx->continuation_state);
     }
-    if (ctx->status == ETS_INSERT_2_LIST_FAILED_TO_GET_LOCK_DESTROY) {
-        return ctx->destroy_return_value;
+    else if (is_non_value(ret)) {
+        ASSERT(p->freason != TRAP);
+        ERTS_BIF_ERROR_TRAPPED2(p, p->freason, BIF_TRAP_EXPORT(bix),
+                                ctx->tid, ctx->list);
     }
     if (ctx->status == ETS_INSERT_2_LIST_GLOBAL) {
         db_unlock(ctx->tb, LCK_WRITE);
     }
-    if (ctx->continuation_state != NULL) {
+    if (do_trap) {
         erts_set_gc_state(p, 0);
         BIF_TRAP2(BIF_TRAP_EXPORT(bix), p, tid, state_mref);
     }
+
+    ASSERT(ret == am_true || ret == am_false);
     return ret;
 }
 
@@ -2433,8 +2504,7 @@ BIF_RETTYPE ets_new_2(BIF_ALIST_2)
     tb->common.status = status;
     tb->common.type = status;
     /* Note, 'type' is *read only* from now on... */
-    tb->common.continuation = NULL;
-    erts_atomic_set_nob(&tb->common.continuation_state, (Sint)NULL);
+    tb->common.continuation_ctx = NULL;
     erts_refc_init(&tb->common.fix_count, 0);
     db_init_lock(tb, status & (DB_FINE_LOCKED|DB_FREQ_READ));
     tb->common.keypos = keypos;
@@ -2629,8 +2699,9 @@ BIF_RETTYPE ets_delete_1(BIF_ALIST_1)
      * Clear all access bits to prevent any ets operation to access the
      * table while it is being deleted.
      */
-    tb->common.status &= ~(DB_PROTECTED|DB_PUBLIC|DB_PRIVATE);
+    tb->common.status &= ~(DB_PROTECTED | DB_PUBLIC | DB_PRIVATE | DB_BUSY);
     tb->common.status |= DB_DELETE;
+    cancel_trap_continuation(tb);
 
     if (tb->common.owner != BIF_P->common.id) {
 
@@ -4660,8 +4731,10 @@ erts_db_process_exiting(Process *c_p, ErtsProcLocks c_p_locks, void **yield_stat
                 break;
             }
             /* Clear all access bits. */
-            tb->common.status &= ~(DB_PROTECTED | DB_PUBLIC | DB_PRIVATE);
+            tb->common.status &= ~(DB_PROTECTED | DB_PUBLIC | DB_PRIVATE
+                                   | DB_BUSY);
             tb->common.status |= DB_DELETE;
+            cancel_trap_continuation(tb);
 
             if (is_table_named(tb))
                 remove_named_tab(tb, 0);
diff --git a/erts/emulator/beam/erl_db_util.h b/erts/emulator/beam/erl_db_util.h
index 6ed1e15104..5289e93199 100644
--- a/erts/emulator/beam/erl_db_util.h
+++ b/erts/emulator/beam/erl_db_util.h
@@ -321,11 +321,7 @@ typedef struct db_table_common {
     int compress;
 
     /* For unfinished operations that needs to be helped */
-    void (*continuation)(long *reds_ptr,
-                         void** state,
-                         void* extra_context); /* To help yielded process */
-    erts_atomic_t continuation_state;
-    Binary* continuation_res_bin;
+    struct ets_insert_2_list_info* continuation_ctx;
 #ifdef ETS_DBG_FORCE_TRAP
     int dbg_force_trap;  /* force trap on table lookup */
 #endif
diff --git a/lib/stdlib/test/ets_SUITE.erl b/lib/stdlib/test/ets_SUITE.erl
index dd8fe43da6..2b0dabc357 100644
--- a/lib/stdlib/test/ets_SUITE.erl
+++ b/lib/stdlib/test/ets_SUITE.erl
@@ -36,6 +36,7 @@
 -export([dups/1, misc1/1, safe_fixtable/1, info/1, tab2list/1]).
 -export([info_binary_stress/1]).
 -export([info_whereis_busy/1]).
+-export([insert_trap_delete/1, insert_trap_rename/1]).
 -export([tab2file/1, tab2file2/1, tabfile_ext1/1,
 	 tabfile_ext2/1, tabfile_ext3/1, tabfile_ext4/1, badfile/1]).
 -export([heavy_lookup/1, heavy_lookup_element/1, heavy_concurrent/1]).
@@ -217,7 +218,9 @@ groups() ->
       [t_insert_list, t_insert_list_set, t_insert_list_bag,
        t_insert_list_duplicate_bag, t_insert_list_delete_set,
        t_insert_list_parallel, t_insert_list_delete_parallel,
-       t_insert_list_kill_process]}].
+       t_insert_list_kill_process,
+       insert_trap_delete,
+       insert_trap_rename]}].
 
 init_per_suite(Config) ->
     erts_debug:set_internal_state(available_internal_state, true),
@@ -1907,7 +1910,7 @@ t_select_replace_next_bug(Config) when is_list(Config) ->
 
 
 %% OTP-17379
-t_select_pam_stack_overflow_bug(Config) ->
+t_select_pam_stack_overflow_bug(_Config) ->
     T = ets:new(k, []),
     ets:insert(T,[{x,17}]),
     [{x,18}] = ets:select(T,[{{x,17}, [], [{{{element,1,'$_'},{const,18}}}]}]),
@@ -5139,6 +5142,184 @@ info_whereis_busy(Config) when is_list(Config) ->
     ets:delete(T),
     ok.
 
+%% Delete table during trapping ets:insert
+insert_trap_delete(Config) when is_list(Config) ->
+    repeat_for_opts(fun(Opts) ->
+                            [insert_trap_delete_run1({Opts,InsertFunc,Mode})
+                             || InsertFunc <- [insert,insert_new],
+                                Mode <- [exit, delete]]
+                    end,
+                    [all_non_stim_types, write_concurrency, compressed]),
+    ok.
+
+insert_trap_delete_run1(Params) ->
+    NKeys = 50_000 + rand:uniform(50_000),
+    %% First measure how many traps the insert op will do
+    Traps0 = insert_trap_delete_run3(unlimited, Params, NKeys),
+    %% Then do again and delete table at different moments
+    Decr = (Traps0 div 5) + 1,
+    insert_trap_delete_run2(Traps0-1, Decr, Params, NKeys).
+
+insert_trap_delete_run2(Traps, _Decr, Params, NKeys) when Traps =< 1 ->
+    insert_trap_delete_run3(1, Params, NKeys),
+    ok;
+insert_trap_delete_run2(Traps, Decr, Params, NKeys) ->
+    insert_trap_delete_run3(Traps, Params, NKeys),
+    insert_trap_delete_run2(Traps - Decr, Decr, Params, NKeys).
+
+insert_trap_delete_run3(Traps, {Opts, InsertFunc, Mode}, NKeys) ->
+    io:format("insert_trap_delete_run(~p, ~p, ~p) NKeys=~p\n",
+              [Traps, InsertFunc, Mode, NKeys]),
+    TabName = insert_trap_delete,
+    Tester = self(),
+    Tuples = [{K} || K <- lists:seq(1,NKeys)],
+
+    OwnerFun =
+        fun() ->
+                erlang:trace(Tester, true, [running]),
+                ets_new(TabName, [named_table, public | Opts]),
+                Tester ! {ets_new, ets:whereis(TabName)},
+                io:format("Wait for ets:~p/2 to yield...\n", [InsertFunc]),
+                GotTraps = repeat_while(
+                  fun(N) ->
+                          case receive_any() of
+                              {trace, Tester, out, {ets,InsertFunc,2}} ->
+                                  case N of
+                                      Traps -> {false, Traps};
+                                      _ -> {true, N+1}
+                                  end;
+                              "Insert done" ->
+                                  io:format("Too late! Got ~p traps\n", [N]),
+                                  {false, N};
+                              _M ->
+                                  %%io:format("[~p] Ignored msg: ~p\n", [N,_M]),
+                                  {true, N}
+                          end
+                  end,
+                  0),
+                case Mode of
+                    delete ->
+                        io:format("Delete table and then exit...\n",[]),
+                        ets:delete(TabName);
+                    exit ->
+                        io:format("Exit and let table die...\n",[])
+                end,
+                Tester ! {traps, GotTraps}
+        end,
+    {Owner, Mon} = spawn_opt(OwnerFun, [link, monitor]),
+
+    {ets_new, Tid} = receive_any(),
+    try ets:InsertFunc(TabName, Tuples) of
+        true ->
+            try ets:lookup(Tid, NKeys) of
+                [{NKeys}] -> ok
+            catch
+                error:badarg ->
+                    %% Table must been deleted just after insert finished
+                    undefined = ets:info(Tid, id),
+                    undefined = ets:whereis(TabName)
+            end,
+            Owner ! "Insert done"
+    catch
+        error:badarg ->
+            %% Insert failed, table must have been deleted
+            undefined = ets:info(Tid, id),
+            undefined = ets:whereis(TabName)
+    end,
+    {traps, GotTraps} = receive_any(),
+    {'DOWN', Mon, process, Owner, _} = receive_any(),
+    undefined = ets:whereis(TabName),
+    undefined = ets:info(Tid, id),
+    GotTraps.
+
+%% Rename table during trapping ets:insert
+insert_trap_rename(Config) when is_list(Config) ->
+    repeat_for_opts(fun(Opts) ->
+                            [insert_trap_rename_run1(InsertFunc)
+                             || InsertFunc <- [insert, insert_new]]
+                    end,
+                    [all_non_stim_types, write_concurrency, compressed]),
+    ok.
+
+insert_trap_rename_run1(InsertFunc) ->
+    NKeys = 50_000 + rand:uniform(50_000),
+    %% First measure how many traps the insert op will do
+    Traps0 = insert_trap_rename_run3(unlimited, InsertFunc, NKeys),
+    %% Then do again and rename table at different moments
+    Decr = (Traps0 div 5) + 1,
+    insert_trap_rename_run2(Traps0-1, Decr, InsertFunc, NKeys),
+    ok.
+
+insert_trap_rename_run2(Traps, _Decr, InsertFunc, NKeys) when Traps =< 1 ->
+    insert_trap_rename_run3(1, InsertFunc, NKeys),
+    ok;
+insert_trap_rename_run2(Traps, Decr, InsertFunc, NKeys) ->
+    insert_trap_rename_run3(Traps, InsertFunc, NKeys),
+    insert_trap_rename_run2(Traps - Decr, Decr, InsertFunc, NKeys).
+
+
+insert_trap_rename_run3(Traps, InsertFunc, NKeys) ->
+    io:format("insert_trap_rename_run(~p, ~p)\n", [Traps, InsertFunc]),
+    TabName = insert_trap_rename,
+    TabRenamed = insert_trap_rename_X,
+    Tester = self(),
+    Tuples = [{K} || K <- lists:seq(1,NKeys)],
+
+    OwnerFun =
+        fun() ->
+                erlang:trace(Tester, true, [running]),
+                ets:new(TabName, [named_table, public]),
+                Tester ! {ets_new, ets:whereis(TabName)},
+                io:format("Wait for ets:~p/2 to yield...\n", [InsertFunc]),
+                GotTraps = repeat_while(
+                  fun(N) ->
+                          case receive_any() of
+                              {trace, Tester, out, {ets,InsertFunc,2}} ->
+                                  case N of
+                                      Traps -> {false, ok};
+                                      _ -> {true, N+1}
+                                  end;
+                              "Insert done" ->
+                                  io:format("Too late! Got ~p traps\n", [N]),
+                                  {false, N};
+                              _M ->
+                                  %%io:format("[~p] Ignored msg: ~p\n", [N,_M]),
+                                  {true, N}
+                          end
+                  end,
+                  0),
+                io:format("Rename table and wait...\n",[]),
+                ets:rename(TabName, TabRenamed),
+                ets:delete(TabRenamed, 42),
+                Tester ! {renamed, GotTraps},
+                receive die -> ok end
+        end,
+    {Owner, Mon} = spawn_opt(OwnerFun, [link,monitor]),
+
+    {ets_new, Tid} = receive_any(),
+    try ets:InsertFunc(TabName, Tuples) of
+        true ->
+            io:format("ets:~p succeeded\n", [InsertFunc]),
+            true = ets:member(Tid, 1),
+            true = ets:member(Tid, NKeys)
+    catch
+        error:badarg ->
+            io:format("ets:~p failed\n", [InsertFunc]),
+            false = ets:member(Tid, 1),
+            false = ets:member(Tid, NKeys)
+    end,
+    Owner ! "Insert done",
+    {renamed, GotTraps} = receive_any(),
+    [] = ets:lookup(Tid, 42),
+    undefined = ets:whereis(TabName),
+    Tid = ets:whereis(TabRenamed),
+    Owner ! die,
+    {'DOWN', Mon, process, Owner, _} = receive_any(),
+    undefined = ets:whereis(TabName),
+    undefined = ets:whereis(TabRenamed),
+    GotTraps.
+
+
 test_table_size_concurrency(Config) when is_list(Config) ->
     case erlang:system_info(schedulers) of
         1 -> {skip,"Only valid on smp > 1 systems"};
@@ -8464,7 +8645,7 @@ repeat_for_permutations(Fun, List, N) ->
 
 receive_any() ->
     receive M ->
-	    io:format("Process ~p got msg ~p\n", [self(),M]),
+	    %%io:format("Process ~p got msg ~p\n", [self(),M]),
 	    M
     end.
 
@@ -8702,6 +8883,8 @@ error_info(_Config) ->
          {insert, ['$Tab', [a|b]]},
          {insert, ['$Tab', {a,b,c}], [no_fail]},
          {insert, ['$Tab', [{a,b,c}]], [no_fail]},
+         {insert, ['$Tab', [{a,b,c},{d,e,f}]], [no_fail]},
+         {insert, ['$Tab', [{I,b,c} || I <- lists:seq(1,10_000)]], [no_fail]},
 
          {insert_new, ['$Tab', bad_object]},
          {insert_new, ['$Tab', {a,b,c}], [no_fail]},
@@ -8860,7 +9043,7 @@ error_info(_Config) ->
         [] ->
             ok;
         [_|_]=Errors ->
-            io:format("~p\n", [Errors]),
+            io:format("~P\n", [Errors, 100]),
             ct:fail({length(Errors),errors})
     end.
 
@@ -9085,7 +9268,7 @@ ets_apply(F, Args, Opts) ->
     end.
 
 ets_format_args(Args) ->
-    lists:join(", ", [io_lib:format("~p", [A]) || A <- Args]).
+    lists:join(", ", [io_lib:format("~P", [A,10]) || A <- Args]).
 
 %%%
 %%% Common utility functions.
-- 
2.35.3

Places

File 0545-erts-Robustify-yielding-ets-insert-2-and-ets-insert_.patch of Package erlang (Revision 3e53aa07e56d6cc0c5ffbbfd2f14b043)

Places