Bug #36824054: scope_guard's use of "commit" and "rollback" confusing and non-standard

Renamed scope_guard::rollback() to scope_guard::reset()
Renamed scope_guard::commit() to scope_guard::release()
Added Doxygen for the class and the functor.

Change-Id: Ided6a71aa468f39eb1f9cea838e42da60ddf2f41

Author: gkodinov

components/libminchassis/dynamic_loader.cc

@@ -698,7 +698,7 @@ bool mysql_dynamic_loader_imp::load_do_load_component_by_scheme(
   bool res = mysql_dynamic_loader_imp::load_do_collect_services_provided(
       loaded_components);
   if (!res) {
-    guard.commit();
+    guard.release();
   }
   return res;
 }
@@ -827,7 +827,7 @@ bool mysql_dynamic_loader_imp::load_do_register_services(
   bool res =
       mysql_dynamic_loader_imp::load_do_resolve_dependencies(loaded_components);
   if (!res) {
-    guard.commit();
+    guard.release();
   }
   return res;
 }
@@ -873,7 +873,7 @@ bool mysql_dynamic_loader_imp::load_do_resolve_dependencies(
   bool res = mysql_dynamic_loader_imp::load_do_initialize_components(
       loaded_components);
   if (!res) {
-    guard.commit();
+    guard.release();
   }
   return res;
 }
@@ -919,7 +919,7 @@ bool mysql_dynamic_loader_imp::load_do_initialize_components(
 
   bool res = mysql_dynamic_loader_imp::load_do_commit(loaded_components);
   if (!res) {
-    guard.commit();
+    guard.release();
   }
   return res;
 }

components/libminchassis/dynamic_loader_scheme_file.cc

@@ -179,8 +179,8 @@ DEFINE_BOOL_METHOD(mysql_dynamic_loader_scheme_file_imp::load,
       return true;
     }
 
-    guard_library.commit();
-    guard_library_set.commit();
+    guard_library.release();
+    guard_library_set.release();
 
     return false;
   } catch (...) {

components/test/test_mysql_thd_store_service.cc

@@ -111,7 +111,7 @@ long long test_thd_store_service_function(UDF_INIT *, UDF_ARGS *,
     return 0;
   }
 
-  cleanup.commit();
+  cleanup.release();
 
   return 1;
 }
@@ -179,7 +179,7 @@ static mysql_service_status_t init() {
   if (mysql_thd_store_service->set(o_thd, g_slot, test_mysql_thd_data))
     return true;
 
-  cleanup_guard.commit();
+  cleanup_guard.release();
 
   return false;
 }

components/test/test_sensitive_system_variables.cc

@@ -141,7 +141,7 @@ test_component_sensitive_system_variables_service_init() {
     return 1;
   }
 
-  cleanup.commit();
+  cleanup.release();
   return 0;
 }
 

include/scope_guard.h

@@ -24,39 +24,91 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301  USA */
 #ifndef SCOPE_GUARD_H
 #define SCOPE_GUARD_H
 
+/**
+  @brief A Lambda to be called at scope exit.
+
+  Used as std::scope_exit of sorts.
+  Useful if you can't use unique_ptr to install a
+  specific deleter but still want to do automatic
+  cleanup at scope exit.
+
+  @note Always use @ref create_scope_guard() instead of this template directly!
+
+  Typical use is:
+  @code
+     ...
+     foo_init();
+     auto cleanup_foo = create_scope_guard([&] {
+        foo_deinit();
+     }
+     ...
+     if (some_error)
+       return; // cleanup_foo calls foo_deinit()
+     ...
+     // foo_deinit is not going to be called past this point
+     cleanup_foo.release();
+     return; // return with foo initialized.
+     ...
+   @endcode
+*/
 template <typename TLambda>
 class Scope_guard {
  public:
-  Scope_guard(const TLambda &rollback_lambda)
-      : m_committed(false), m_rollback_lambda(rollback_lambda) {}
+  Scope_guard(const TLambda &cleanup_lambda)
+      : m_is_released(false), m_cleanup_lambda(cleanup_lambda) {}
   Scope_guard(const Scope_guard<TLambda> &) = delete;
   Scope_guard(Scope_guard<TLambda> &&moved)
-      : m_committed(moved.m_committed),
-        m_rollback_lambda(moved.m_rollback_lambda) {
-    /* Set moved guard to "invalid" state, the one in which the rollback lambda
+      : m_is_released(moved.m_is_released),
+        m_cleanup_lambda(moved.m_cleanup_lambda) {
+    /* Set moved guard to "invalid" state, the one in which the cleanup lambda
       will not be executed. */
-    moved.m_committed = true;
+    moved.m_is_released = true;
   }
   ~Scope_guard() {
-    if (!m_committed) {
-      m_rollback_lambda();
+    if (!m_is_released) {
+      m_cleanup_lambda();
     }
   }
 
-  inline void commit() { m_committed = true; }
+  /**
+    @brief Releases the scope guard
+
+    Makes sure that when scope guard goes out of scope the
+    cleanup lambda is not going to be called.
+  */
+  inline void release() { m_is_released = true; }
 
-  inline void rollback() {
-    if (!m_committed) {
-      m_rollback_lambda();
-      m_committed = true;
+  /**
+    @brief Calls the cleanup lambda and releases the scope guard
+
+    Useful if you want to explicitly provoke the cleanup earlier
+    than when going out of scope.
+  */
+  inline void reset() {
+    if (!m_is_released) {
+      m_cleanup_lambda();
+      m_is_released = true;
     }
   }
 
  private:
-  bool m_committed;
-  const TLambda m_rollback_lambda;
+  /** If true the cleanup is not going to be called */
+  bool m_is_released;
+  /** The cleanup to be called */
+  const TLambda m_cleanup_lambda;
 };
 
+/**
+  @brief Create a scope guard object
+
+  Always use this instead of the @ref Scope_guard template itself!
+  @sa @ref Scope_guard
+
+  @tparam TLambda The type of the lambda. Inferred, never specify it.
+  @param rollback_lambda The lambda to execute.
+  @return Scope_guard<TLambda> An specialization of the @ref Scope_guard
+  template.
+*/
 template <typename TLambda>
 Scope_guard<TLambda> create_scope_guard(const TLambda rollback_lambda) {
   return Scope_guard<TLambda>(rollback_lambda);

mysys/my_winfile.cc

@@ -684,7 +684,7 @@ FILE *my_win_fopen(const char *filename, const char *mode) {
                      flags) < 0)
     return nullptr;
 
-  sg.commit();  // Do not close the stream we are about to return
+  sg.release();  // Do not close the stream we are about to return
   return stream;
 }
 
@@ -784,8 +784,8 @@ FILE *my_win_freopen(const char *path, const char *mode, FILE *stream) {
   if (_dup2(handle_fd, fd) < 0) return nullptr;
 
   // Leave the handle and fd open, but close handle_fd
-  chg.commit();
-  cfdg.commit();
+  chg.release();
+  cfdg.release();
 
   return stream;
 }

plugin/keyring_udf/keyring_udf.cc

@@ -427,7 +427,7 @@ static bool fetch(const char *function_name, char *key_id, char **a_key,
 
   if (a_key_len != nullptr) *a_key_len = fetched_key_len;
 
-  cleanup_guard.commit();
+  cleanup_guard.release();
 
   return false;
 }

plugin/x/src/module_mysqlx.cc

@@ -193,7 +193,7 @@ int Module_mysqlx::initialize(MYSQL_PLUGIN plugin_handle) {
       m_server->delayed_start_tasks();
     }
 
-    guard_of_server_start.commit();
+    guard_of_server_start.release();
   } catch (const std::exception &e) {
     log_error(ER_XPLUGIN_STARTUP_FAILED, e.what());
     return 1;

router/src/connection_pool/src/connection_pool_plugin.cc

@@ -131,7 +131,7 @@ static void init(mysql_harness::PluginFuncEnv *env) {
               std::chrono::seconds{config.idle_timeout}));
     }
 
-    init_guard.commit();
+    init_guard.release();
   } catch (const std::invalid_argument &exc) {
     set_error(env, mysql_harness::kConfigInvalidArgument, "%s", exc.what());
 

router/src/http/src/http_server_plugin.cc

@@ -610,7 +610,7 @@ static void init(mysql_harness::PluginFuncEnv *env) {
                                config.static_basedir, config.require_realm));
       }
 
-      initialization_finished.commit();
+      initialization_finished.release();
     }
   } catch (const std::invalid_argument &exc) {
     set_error(env, mysql_harness::kConfigInvalidArgument, "%s", exc.what());

router/src/routing/src/mysql_routing.cc

@@ -849,7 +849,7 @@ stdx::expected<void, std::string> MySQLRouting::run_acceptor(
   is_running_ = false;
   get_context().shared_quarantine().stop();
 
-  stop_acceptors_guard.commit();
+  stop_acceptors_guard.release();
   // routing is no longer running, lets close listening socket
   stop_socket_acceptors();
 

sql/histograms/histogram.cc

@@ -1226,7 +1226,7 @@ static bool fill_value_maps(
   if (res != HA_ERR_END_OF_FILE) return true; /* purecov: deadcode */
 
   // Close the handler
-  handler_guard.commit();
+  handler_guard.release();
   if (table->file->ha_sample_end(scan_ctx)) {
     assert(false); /* purecov: deadcode */
     return true;
@@ -1407,7 +1407,7 @@ bool update_histogram(THD *thd, Table_ref *table, const columns_set &columns,
 
     bool ret = trans_commit_stmt(thd) || trans_commit(thd);
     close_thread_tables(thd);
-    tables_guard.commit();
+    tables_guard.release();
 
     return ret;
   }
@@ -1480,7 +1480,7 @@ bool update_histogram(THD *thd, Table_ref *table, const columns_set &columns,
 
   bool ret = trans_commit_stmt(thd) || trans_commit(thd);
   close_thread_tables(thd);
-  tables_guard.commit();
+  tables_guard.release();
   return ret;
 }
 

sql/iterators/composite_iterators.cc

@@ -931,7 +931,7 @@ bool MaterializeIterator<Profiler>::Init() {
     }
   } else {
     // We didn't open the index, so we don't need to close it.
-    end_unique_index.commit();
+    end_unique_index.release();
   }
   ha_rows stored_rows = 0;
 
@@ -953,7 +953,7 @@ bool MaterializeIterator<Profiler>::Init() {
     }
   }
 
-  end_unique_index.rollback();
+  end_unique_index.reset();
   table()->materialized = true;
 
   if (!m_rematerialize) {
@@ -1787,7 +1787,7 @@ bool TemptableAggregateIterator<Profiler>::Init() {
       */
       if (error != 0 && error != HA_ERR_RECORD_IS_THE_SAME) {
         if (move_table_to_disk(error, /*insert_operation=*/false)) {
-          end_unique_index.commit();
+          end_unique_index.release();
           return true;
         }
         /*
@@ -1888,7 +1888,7 @@ bool TemptableAggregateIterator<Profiler>::Init() {
       }
 
       if (move_table_to_disk(error, /*insert_operation=*/true)) {
-        end_unique_index.commit();
+        end_unique_index.release();
         return true;
       }
     } else {
@@ -1898,7 +1898,7 @@ bool TemptableAggregateIterator<Profiler>::Init() {
   }
 
   table()->file->ha_index_end();
-  end_unique_index.commit();
+  end_unique_index.release();
 
   table()->materialized = true;
 

sql/server_component/persistent_dynamic_loader.cc

@@ -441,8 +441,8 @@ DEFINE_BOOL_METHOD(mysql_persistent_dynamic_loader_imp::load,
       return true;
     }
 
-    guard.commit();
-    guard_close_tables.commit();
+    guard.release();
+    guard_close_tables.release();
     trans_commit_stmt(thd);
     return false;
   } catch (...) {
@@ -551,7 +551,7 @@ DEFINE_BOOL_METHOD(mysql_persistent_dynamic_loader_imp::unload,
       return result;
     }
 
-    guard_close_tables.commit();
+    guard_close_tables.release();
     trans_commit_stmt(thd);
     return false;
   } catch (...) {

sql/sql_cmd_srs.cc

@@ -212,7 +212,7 @@ bool Sql_cmd_create_srs::execute(THD *thd) {
 
     if (dd_client->update(srs)) return true;  // Error has already been flagged.
 
-    rollback_guard.commit();
+    rollback_guard.release();
     if (commit(thd)) return true; /* purecov: inspected */
     my_ok(thd);
     return false;
@@ -228,7 +228,7 @@ bool Sql_cmd_create_srs::execute(THD *thd) {
 
   if (thd->dd_client()->store(new_srs.get())) return true;
 
-  rollback_guard.commit();
+  rollback_guard.release();
   if (commit(thd)) return true; /* purecov: inspected */
   my_ok(thd);
   return false;
@@ -274,7 +274,7 @@ bool Sql_cmd_drop_srs::execute(THD *thd) {
   }
 
   if (dd_client->drop(srs)) return true; /* purecov: inspected */
-  rollback_guard.commit();
+  rollback_guard.release();
   if (commit(thd)) return true; /* purecov: inspected */
   my_ok(thd);
   return false;

sql/sql_prepare.cc

@@ -3276,7 +3276,7 @@ bool Prepared_statement::reprepare(THD *thd) {
   */
   thd->get_stmt_da()->reset_condition_info(thd);
 
-  copy_guard.commit();
+  copy_guard.release();
 
   return false;
 }

sql/sql_select.cc

@@ -4909,7 +4909,7 @@ bool JOIN::make_tmp_tables_info() {
     etc).
   */
   assert(!query_block->is_recursive() || !tmp_tables);
-  cleanup_tmp_tables_on_error.commit();
+  cleanup_tmp_tables_on_error.release();
   return false;
 }
 

sql/sql_table.cc

@@ -11651,7 +11651,7 @@ bool Sql_cmd_secondary_load_unload::mysql_secondary_load_or_unload(
              ("commit succeeded for alter table %s secondary_%s", full_tab_name,
               (is_load ? "load" : "unload")));
   // Transaction committed successfully, no rollback will be necessary.
-  rollback_guard.commit();
+  rollback_guard.release();
 
   if (cleanup()) return true;