#At file:///export/home/didrik/mysqldev/new-part2-mdl/ based on revid:tor.didriksen@stripped
2814 Tor Didriksen 2009-09-08
WL#5077
Fixup according comments from reviewer, mostly more comments.
modified:
sql/mdl-t.cc
sql/mdl_test.cc
sql/sql_list_test.cc
sql/thread.cc
sql/thread.h
sql/thread_test.cc
=== modified file 'sql/mdl-t.cc'
--- a/sql/mdl-t.cc 2009-09-02 08:47:33 +0000
+++ b/sql/mdl-t.cc 2009-09-08 11:38:34 +0000
@@ -206,7 +206,6 @@ protected:
void concurrent_shared_exclusive();
void concurrent_exclusive_shared();
void concurrent_upgrade();
- void expect_failures();
THD *m_thd;
const MDL_ticket *m_null_ticket;
@@ -230,7 +229,7 @@ public:
: m_mdl_type(mdl_type),
m_lock_grabbed(lock_grabbed),
m_release_locks(release_locks),
- m_thd((THD*)this) // See notify_thread below.
+ m_thd(reinterpret_cast<THD*>(this)) // See notify_thread below.
{
m_mdl_context.init(m_thd);
}
@@ -464,18 +463,23 @@ TEST_F(MDL_test, upgrade_exclusive)
MDL_request *request= create_request(table_name1);
request->set_type(MDL_EXCLUSIVE);
EXPECT_FALSE(m_mdl_context.try_acquire_exclusive_lock(request));
+ EXPECT_NE(m_null_ticket, request->ticket);
+ EXPECT_FALSE(request->ticket->is_shared());
EXPECT_FALSE(request->ticket->upgrade_shared_lock_to_exclusive());
+ EXPECT_FALSE(request->ticket->is_shared());
m_mdl_context.release_lock(request->ticket);
}
-// Disable this test, since assert-fails in mdl_destroy().
-// (gdb) p global_lock
-// $1 = {waiting_shared = 0,
-// active_shared = 0,
-// active_intention_exclusive = 4294967295}
-// The upgrade from SHARED to EXCLUSIVE should fail, but it does not,
-// and something is wrong with the maintenance of active_intention_exclusive.
+/*
+ Disable this test, since assert-fails in mdl_destroy().
+ (gdb) p global_lock
+ $1 = {waiting_shared = 0,
+ active_shared = 0,
+ active_intention_exclusive = 4294967295}
+ The upgrade from SHARED to EXCLUSIVE should fail, but it does not,
+ and something is wrong with the maintenance of active_intention_exclusive.
+ */
TEST_F(MDL_test, DISABLED_upgrade_shared)
{
MDL_request *request1= create_request(table_name1);
@@ -650,17 +654,6 @@ TEST_F(MDL_test, concurrent_upgrade)
m_mdl_context.release_all_locks();
}
-int return_one()
-{
- return 1;
-}
-
-TEST_F(MDL_test, expect_failures)
-{
- EXPECT_TRUE(1 == 0);
- EXPECT_EQ(0, return_one());
-}
-
} // namespace
@@ -697,7 +690,6 @@ int MDL_test::RUN_ALL_TESTS()
run_one_test(&MDL_test::concurrent_shared_exclusive);
run_one_test(&MDL_test::concurrent_exclusive_shared);
run_one_test(&MDL_test::concurrent_upgrade);
- run_one_test(&MDL_test::expect_failures);
// Execute MDL_test::TearDownTestCase() here, if it is defined.
return exit_status();
=== modified file 'sql/mdl_test.cc'
--- a/sql/mdl_test.cc 2009-09-02 08:47:33 +0000
+++ b/sql/mdl_test.cc 2009-09-08 11:38:34 +0000
@@ -44,7 +44,12 @@ namespace {
bool notify_thread(THD*);
}
-// We need to mock away this global function.
+/*
+ We need to mock away this global function, because the real version
+ pulls in a lot of dependencies.
+ (The @note for the real version of this function indicates that the
+ coupling between THD and MDL is too tight.)
+*/
extern bool mysql_notify_thread_having_shared_lock(THD *thd, THD *in_use)
{
if (in_use != NULL)
@@ -52,13 +57,20 @@ extern bool mysql_notify_thread_having_s
return false;
}
-// We need to mock away this global function.
+/*
+ Mock away this function as well, with an empty function.
+ TODO(didrik): Consider verifying that the MDL module actually calls
+ this with correct arguments.
+*/
extern void mysql_ha_flush(THD *)
{
DBUG_PRINT("mysql_ha_flush", ("mock version"));
}
-// We need to mock away this global function.
+/*
+ We need to mock away this global function, the real version pulls in
+ too many dependencies.
+ */
extern "C" const char *set_thd_proc_info(THD *thd, const char *info,
const char *calling_function,
const char *calling_file,
@@ -69,7 +81,10 @@ extern "C" const char *set_thd_proc_info
return info;
}
-// We need to mock away this global function.
+/*
+ Mock away this global function.
+ We don't need DEBUG_SYNC functionality in a unit test.
+ */
void debug_sync(THD *thd, const char *sync_point_name, size_t name_len)
{
DBUG_PRINT("debug_sync_point", ("hit: '%s'", sync_point_name));
@@ -139,7 +154,7 @@ public:
: m_mdl_type(mdl_type),
m_lock_grabbed(lock_grabbed),
m_release_locks(release_locks),
- m_thd((THD*)this) // See notify_thread below.
+ m_thd(reinterpret_cast<THD*>(this)) // See notify_thread below.
{
m_mdl_context.init(m_thd);
}
@@ -205,6 +220,9 @@ bool is_lock_owner(MDL_context *context,
}
+/*
+ Verifies that we die with a DBUG_ASSERT if we destry a non-empty MDL_context.
+ */
TEST_F(MDL_DeathTest, die_when_m_tickets_nonempty)
{
::testing::FLAGS_gtest_death_test_style = "threadsafe";
@@ -217,6 +235,10 @@ TEST_F(MDL_DeathTest, die_when_m_tickets
}
+/*
+ Verifies that we die with a DBUG_ASSERT if we destry a MDL_context
+ while holding the global shared lock.
+ */
TEST_F(MDL_DeathTest, die_when_holding_global_shared_lock)
{
::testing::FLAGS_gtest_death_test_style = "threadsafe";
@@ -229,11 +251,17 @@ TEST_F(MDL_DeathTest, die_when_holding_g
}
+/*
+ The most basic test: just construct and destruct our test fixture.
+ */
TEST_F(MDL_test, construct_and_destruct)
{
}
+/*
+ Acquires one lock of type MDL_SHARED.
+ */
TEST_F(MDL_test, one_shared)
{
MDL_request *request= create_request(table_name1);
@@ -254,6 +282,9 @@ TEST_F(MDL_test, one_shared)
}
+/*
+ Acquires one lock of type MDL_SHARED_HIGH_PRIO.
+ */
TEST_F(MDL_test, one_shared_high_prio)
{
MDL_request *request= create_request(table_name1);
@@ -274,6 +305,9 @@ TEST_F(MDL_test, one_shared_high_prio)
}
+/*
+ Acquires one lock of type MDL_SHARED_UPGRADABLE.
+ */
TEST_F(MDL_test, one_shared_upgradable)
{
MDL_request *request= create_request(table_name1);
@@ -294,6 +328,9 @@ TEST_F(MDL_test, one_shared_upgradable)
}
+/*
+ Acquires one lock of type MDL_EXCLUSIVE.
+ */
TEST_F(MDL_test, one_exclusive)
{
MDL_request *request= create_request(table_name1);
@@ -314,6 +351,10 @@ TEST_F(MDL_test, one_exclusive)
}
+/*
+ Acquires wo locks of type MDL_SHARED.
+ Verifies that they are independent.
+ */
TEST_F(MDL_test, two_shared)
{
MDL_request *request1= create_request(table_name1);
@@ -343,6 +384,10 @@ TEST_F(MDL_test, two_shared)
}
+/*
+ Verifies that two different contexts can acquire a shared lock
+ on the same table.
+ */
TEST_F(MDL_test, shared_locks_between_contexts)
{
THD *thd2= (THD*) 0x1;
@@ -362,6 +407,9 @@ TEST_F(MDL_test, shared_locks_between_co
}
+/*
+ Verifies that we can upgrade a shared lock to exclusive.
+ */
TEST_F(MDL_test, upgrade_shared_upgradable)
{
MDL_request *request= create_request(table_name1);
@@ -372,23 +420,32 @@ TEST_F(MDL_test, upgrade_shared_upgradab
}
+/*
+ Verifies that we can grab an exclusive lock, and that it is OK to try
+ to upgrade it to exclusive.
+ */
TEST_F(MDL_test, upgrade_exclusive)
{
MDL_request *request= create_request(table_name1);
request->set_type(MDL_EXCLUSIVE);
EXPECT_FALSE(m_mdl_context.try_acquire_exclusive_lock(request));
+ EXPECT_NE(m_null_ticket, request->ticket);
+ EXPECT_FALSE(request->ticket->is_shared());
EXPECT_FALSE(request->ticket->upgrade_shared_lock_to_exclusive());
+ EXPECT_FALSE(request->ticket->is_shared());
m_mdl_context.release_lock(request->ticket);
}
-// Disable this test, since assert-fails in mdl_destroy().
-// (gdb) p global_lock
-// $1 = {waiting_shared = 0,
-// active_shared = 0,
-// active_intention_exclusive = 4294967295}
-// The upgrade from SHARED to EXCLUSIVE should fail, but it does not,
-// and something is wrong with the maintenance of active_intention_exclusive.
+/*
+ Disable this test, since assert-fails in mdl_destroy().
+ (gdb) p global_lock
+ $1 = {waiting_shared = 0,
+ active_shared = 0,
+ active_intention_exclusive = 4294967295}
+ The upgrade from SHARED to EXCLUSIVE should fail, but it does not,
+ and something is wrong with the maintenance of active_intention_exclusive.
+ */
TEST_F(MDL_test, DISABLED_upgrade_shared)
{
MDL_request *request1= create_request(table_name1);
@@ -405,6 +462,10 @@ TEST_F(MDL_test, DISABLED_upgrade_shared
}
+/*
+ Verifies that we can grab locks in different contexts, and then merge
+ the locks into one context (releasing them from the other).
+ */
TEST_F(MDL_test, merge)
{
MDL_request *request1= create_request(table_name1);
@@ -439,6 +500,9 @@ TEST_F(MDL_test, merge)
}
+/*
+ Verfies that locks are released when we roll back to a savepoint.
+ */
TEST_F(MDL_test, savepoint)
{
MDL_request *request1= create_request(table_name1);
@@ -467,6 +531,9 @@ TEST_F(MDL_test, savepoint)
}
+/*
+ Verifies that we can grab shared locks concurrently, in different threads.
+ */
TEST_F(MDL_test, concurrent_shared)
{
Notification lock_grabbed;
@@ -488,6 +555,10 @@ TEST_F(MDL_test, concurrent_shared)
}
+/*
+ Verifies that we cannot grab an exclusive lock on something which
+ is locked with a shared lock in a different thread.
+ */
TEST_F(MDL_test, concurrent_shared_exclusive)
{
Notification lock_grabbed;
@@ -514,6 +585,10 @@ TEST_F(MDL_test, concurrent_shared_exclu
}
+/*
+ Verifies that we cannot we cannot grab a shared lock on something which
+ is locked exlusively in a different thread.
+ */
TEST_F(MDL_test, concurrent_exclusive_shared)
{
Notification lock_grabbed;
@@ -542,6 +617,14 @@ TEST_F(MDL_test, concurrent_exclusive_sh
}
+/*
+ Verifies the following scenario:
+ Thread 1: grabs a shared upgradable lock.
+ Thread 2: grabs a shared lock.
+ Thread 1: asks for an upgrade to exclusive (needs to wait for thread 2)
+ Thread 2: gets notified, and releases lock.
+ Thread 1: gets the exclusive lock.
+ */
TEST_F(MDL_test, concurrent_upgrade)
{
MDL_request request;
@@ -563,17 +646,6 @@ TEST_F(MDL_test, concurrent_upgrade)
m_mdl_context.release_all_locks();
}
-int return_one()
-{
- return 1;
-}
-
-TEST_F(MDL_test, expect_failures)
-{
- EXPECT_TRUE(1 == 0) << "I expected this to be true!";
- EXPECT_EQ(0, return_one()) << "I expected these to be equal!";
-}
-
} // namespace
int main(int argc, char **argv) {
=== modified file 'sql/sql_list_test.cc'
--- a/sql/sql_list_test.cc 2009-09-02 08:03:35 +0000
+++ b/sql/sql_list_test.cc 2009-09-08 11:38:34 +0000
@@ -76,6 +76,8 @@ protected:
init_sql_alloc(&m_mem_root, 1 << 10, 0);
MEM_ROOT *pm= &m_mem_root;
ASSERT_EQ(0, my_pthread_setspecific_ptr(THR_MALLOC, &pm));
+ MEM_ROOT *root= *my_pthread_getspecific_ptr(MEM_ROOT**,THR_MALLOC);
+ ASSERT_EQ(root, pm);
}
virtual void TearDown()
=== modified file 'sql/thread.cc'
--- a/sql/thread.cc 2009-09-02 08:47:33 +0000
+++ b/sql/thread.cc 2009-09-08 11:38:34 +0000
@@ -17,12 +17,14 @@
namespace thread {
+namespace {
void *thread_start_routine(void *arg)
{
Thread *thread= (Thread*) arg;
Thread::run_wrapper(thread);
return NULL;
}
+} // namespace
Thread::~Thread()
{
=== modified file 'sql/thread.h'
--- a/sql/thread.h 2009-09-02 08:47:33 +0000
+++ b/sql/thread.h 2009-09-08 11:38:34 +0000
@@ -19,10 +19,6 @@
#include <my_global.h>
#include <my_pthread.h>
-#define DISALLOW_COPY_AND_ASSIGN(type)\
- type(const type &);\
- void operator=(const type &)
-
namespace thread {
/*
@@ -84,7 +80,8 @@ private:
pthread_t m_thread_id;
Options m_options;
- DISALLOW_COPY_AND_ASSIGN(Thread);
+ Thread(const Thread&); /* Not copyable. */
+ void operator=(const Thread&); /* Not assignable. */
};
@@ -98,8 +95,8 @@ public:
private:
pthread_mutex_t *m_mutex;
- Mutex_lock();
- DISALLOW_COPY_AND_ASSIGN(Mutex_lock);
+ Mutex_lock(const Mutex_lock&); /* Not copyable. */
+ void operator=(const Mutex_lock&); /* Not assignable. */
};
@@ -118,7 +115,8 @@ private:
pthread_cond_t m_cond;
pthread_mutex_t m_mutex;
- DISALLOW_COPY_AND_ASSIGN(Notification);
+ Notification(const Notification&); /* Not copyable. */
+ void operator=(const Notification&); /* Not assignable. */
};
} // namespace thread
=== modified file 'sql/thread_test.cc'
--- a/sql/thread_test.cc 2009-09-02 08:47:33 +0000
+++ b/sql/thread_test.cc 2009-09-08 11:38:34 +0000
@@ -54,10 +54,14 @@ private:
Notification *m_start_notification;
Notification *m_end_notification;
- DISALLOW_COPY_AND_ASSIGN(Notification_thread);
+ Notification_thread(const Notification_thread&); // Not copyable.
+ void operator=(const Notification_thread&); // Not assignable.
};
+/*
+ A basic, single-threaded test of Notification.
+ */
TEST(Notification, notify)
{
Notification notification;
@@ -66,6 +70,10 @@ TEST(Notification, notify)
EXPECT_TRUE(notification.has_been_notified());
}
+/*
+ Starts a thread, and verifies that the notification/synchronization
+ mechanism works.
+ */
TEST(Notification_thread, start_and_wait)
{
Notification start_notification;
Attachment: [text/bzr-bundle] bzr/tor.didriksen@sun.com-20090908113834-731azpl37yey8dxm.bundle
| Thread |
|---|
| • bzr commit into mysql-5.4 branch (tor.didriksen:2814) WL#5077 | Tor Didriksen | 8 Sep |
