失敗ロック例いくつか

なにかあまりスレッドとか得意でない人のコードを見ていて、いくつかダメな予感がするパターンがあるよね、ってことで適当に集めてみました。どれもこれも小さな例にすると、こんなミスしねーよ、って感じなんですけど、複雑なコードの中にあると結構ミスるもんかな、と。私自身マルチスレッドはたいへん苦手で、実際私がやらかしたケースもいくつか。

ひとつめ: ロック順序逆転

// そこらじゅうで確保されてるグローバルなロック
pthread_mutex_t g_mu = PTHREAD_MUTEX_INITIALIZER;

// このクラスを使うところは全域 g_mu でロックされてるとします
class C {
public:
  C() {
    pthread_mutex_init(&mu_, NULL);
  }
  void doSlowOperation() {
    pthread_mutex_lock(&mu_);
    // この処理は重いから先にロックを解放しておこう!
    pthread_mutex_unlock(&g_mu);
    // Slow operation.
    pthread_mutex_lock(&g_mu);
    pthread_mutex_unlock(&mu_);
  }
private:
  pthread_mutex_t mu_;
};

使っている例: https://github.com/shinh/test/blob/master/badlock1.cc

このコードは thread A が出口付近で g_mu を取ろうとする時に、他のスレッドが入口付近で mu_ を確保しにいくと、デッドロックします。

教訓としては、複数ロックがネストする時はとりあえずデッドロックを心配すると良い気がします。ネストする場合は、二つのロックを取る順序は必ず同じ順序で取るようにするべき。ロック1とロック2がある時に、ロック1,2両方取られている時間と、ロック1だけが取られている時間と、ロック2だけが取られている時間があると、まぁだいたいやばいです。

まぁ、この場合は本当に g_mu を外さにゃいかんのかをまず考えた方がいい気がします。あちこちで確保されてるロックが一つだけ、って状態は性能に問題が出るまでは割と安心な状態です。

ふたつめ: 同じ変数に別のロックを使う

// そこらじゅうで確保されてるグローバルなロック
pthread_mutex_t g_mu = PTHREAD_MUTEX_INITIALIZER;

// このクラスを使うところは全域 g_mu でロックされてるとします
public:
  C() {
    pthread_mutex_init(&mu_, NULL);
  }
  void doSlowOperation() {
    // この処理は重いから先にロックを解放しておこう
    // ひとつめではデッドロックしてしまったから先に外しておく
    pthread_mutex_unlock(&g_mu);
    pthread_mutex_lock(&mu_);
    for (int i = 0; i < 5; i++)
      vals_.push_back(i);
    pthread_mutex_unlock(&mu_);
    pthread_mutex_lock(&g_mu);
  }
  void doFastOperation() {
    // g_mu が取られてるから大丈夫!
    vals_.push_back(42);
  }
private:
  pthread_mutex_t mu_;
  std::vector<int> vals_;
};

使っている例: https://github.com/shinh/test/blob/master/badlock2.cc

doSlowOperation を見る限り、 vals_ は mu_ で守られてるべきっぽいですが、 doFastOperation では g_mu に頼ってます。普通に race condition があるので、この場合普通にクラッシュしました。

教訓としては、なにかロックしてるから大丈夫、ではなくて、適切なロックを使ってるかどうかを常に意識すると良いです。どのロックに守られてるか、ってのは C++ の所有権並に重要なことだと思うんで、 mutex をメンバに持たせる時は、その mutex がどの変数をガードするかどうかをコメントするようにするとやや良い気がします。

みっつめ: ロック順序逆転してない?

class C {
public:
  explicit C(int val) : val_(val) {
    pthread_mutex_init(&mu_, NULL);
  }
  int add(C* c) {
    // 必ず mu_ => c->mu_ の順序でロックするよ!
    pthread_mutex_lock(&mu_);
    pthread_mutex_lock(&c->mu_);
    int r = val_ + c->val_;
    pthread_mutex_unlock(&c->mu_);
    pthread_mutex_unlock(&mu_);
    return r;
  }
private:
  pthread_mutex_t mu_;
  int val_;
};

使っている例: https://github.com/shinh/test/blob/master/badlock3.cc

C::add はぱっと見首尾一貫した順序を守ってるように見えるんですが、 this と引数 c が逆転すればひとつめの例と同じで、ロック順序が逆転しているのでデッドロックします。

具体的には、 c1->add(c2) と c2->add(c1) があったとして、前者が c1 のロックを取る→後者が c2 のロックを取る、という順序で起きると次のロックはどちらも取れません。

これはこのクラスの使い方によっては、問題無い場合もあります。必ず一定の法則に従って add のレシーバと引数を選んでいれば大丈夫で、たぶん、

assert(c1 != c2);
c1 < c2 ? c1->add(c2) : c2->add(c1);

とかしていれば大丈夫…な気がします。自信がないですが。

がまぁ素直になんとかした方がいいと思います。パフォーマンスに問題が無ければ mu_ を static にしてしまうのがラクチンです。

よっつめ: 狭義のレースは無いのだけど

class C {
public:
  explicit C() {
    pthread_mutex_init(&mu_, NULL);
  }
  void setVals(std::vector<int> vals) {
    pthread_mutex_lock(&mu_);
    vals_ = vals;
    pthread_mutex_unlock(&mu_);
    // Update the cache.
    setSum(calcSum());
  }
  int getSum() {
    pthread_mutex_lock(&mu_);
    int sum = sum_;
    pthread_mutex_unlock(&mu_);
    return sum;
  }
  void setSum(int sum) {
    pthread_mutex_lock(&mu_);
    sum_ = sum;
    pthread_mutex_unlock(&mu_);
  }
  void check() {
    pthread_mutex_lock(&mu_);
    int sum = 0;
    for (size_t i = 0; i < vals_.size(); i++)
      sum += vals_[i];
    assert(sum == sum_);
    pthread_mutex_unlock(&mu_);
  }
private:
  int calcSum() {
    pthread_mutex_lock(&mu_);
    int sum = 0;
    for (size_t i = 0; i < vals_.size(); i++)
      sum += vals_[i];
    pthread_mutex_unlock(&mu_);
    return sum;
  }
  pthread_mutex_t mu_;
  std::vector<int> vals_;
  int sum_;
};

使っている例: https://github.com/shinh/test/blob/master/badlock4.cc

vector を持っていて、その合計値がよく欲しくなるので、合計値をキャッシュとして持っておく、というような想定で書かれたコードです。ロックが一つしか無くて、 vals_ も sum_ も mu_ にしっかりガードされてるので安心…ではないです。

たしかに vals_ や sum_ に同時アクセスするケースは無いのですが、 sum_ が vals_ の合計でない値になってしまう場合があります。というのは setVals が calcSum や setSum でいちいちロックを外してしまっているので、例えば、 calcSum が 30 という結果を返す→ setVals が別スレッドで呼ばれて sum_ が 50 にアップデートされる→元のスレッドに戻って sum_ に 30 を代入する、のような。

この手のレースはなんと呼ぶのか知らないのですが、一つの変数へのアクセスはちゃんとガードされてるので、 thread sanitizer なんかの race detector にかからず、 annotation をしっかり書いても見つけてくれないはずです。 QuickCheck なんかがこういうのを探してくれる、って話だったような。

この手の、メンバ変数 A が○○なら B は必ずこれを満たさなければならない…みたいな条件があるクラスはマルチスレッドでなくてもあまり嬉しくないですね。パフォーマンスに問題なければそもそもキャッシュしたくないわけですが、まぁこの例ではパフォーマンスに問題があったから calcSum を private に移してキャッシュすることにしたんだなぁ、という流れが透けて見える感じです。 check みたいな関数を用意してやたら呼びまくるのは安心感があっていい方法ではあると思います。

ちなみに修正例:

class C {
public:
  explicit C() : sum_(0) {
    pthread_mutex_init(&mu_, NULL);
  }
  void setVals(std::vector<int> vals) {
    pthread_mutex_lock(&mu_);
    vals_ = vals;
    // Update the cache.
    sum_ = calcSumLocked();
    pthread_mutex_unlock(&mu_);
  }
  int getSum() {
    pthread_mutex_lock(&mu_);
    int sum = sum_;
    pthread_mutex_unlock(&mu_);
    return sum;
  }
  void check() {
    pthread_mutex_lock(&mu_);
    int sum = 0;
    for (size_t i = 0; i < vals_.size(); i++)
      sum += vals_[i];
    assert(sum == sum_);
    pthread_mutex_unlock(&mu_);
  }
private:
  int calcSumLocked() {
    int sum = 0;
    for (size_t i = 0; i < vals_.size(); i++)
      sum += vals_[i];
    return sum;
  }
  pthread_mutex_t mu_;
  std::vector<int> vals_;
  int sum_;
};

こういう hogehogeLocked みたいなのは、こういうクラスだとよく必要になる気がします。

いつつめ: rwlock

最後ふたつは mutex よりは使用頻度が低いもので。

class C {
public:
  C() : val_(3) {
    pthread_rwlockattr_t attr;
    pthread_rwlockattr_init(&attr);
    pthread_rwlockattr_setkind_np(
        &attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP);
    pthread_rwlock_init(&mu_, &attr);
    pthread_rwlockattr_destroy(&attr);
  }
  int calcSomething() {
    pthread_rwlock_rdlock(&mu_);
    // getVal の中で同じロックを取るけど、 reader lock だから大丈夫!
    int r = 42 + getVal();
    pthread_rwlock_unlock(&mu_);
    return r;
  }
  int getVal() {
    pthread_rwlock_rdlock(&mu_);
    int r = val_;
    pthread_rwlock_unlock(&mu_);
    return r;
  }
  void setVal(int val) {
    pthread_rwlock_wrlock(&mu_);
    val_ = val;
    pthread_rwlock_unlock(&mu_);
  }
private:
  pthread_rwlock_t mu_;
  int val_;
};

使っている例: https://github.com/shinh/test/blob/master/badlock5.cc

以前書いた話 です。 calcSomething の中でヘルパ関数 getVal を呼んでいるのは、 glibc のデフォルトの pthread_rwlock_t なら大丈夫なんですが、この場合 writer starvation を回避する rwlockattr をつけてるのでデッドロックしてしまいます。また、 pthread の実装によってはデフォルトのでデッドロックするそうです。

writer starvation とは、 reader がたくさんいて、一つのロックに常に複数の reader lock を reader が取ってしまっていて、全然 writer がロックを取れない状況を言うそうです。これを回避するために、 writer が待ってる時は新規 reader を受け付けない、という処置がされているロックがあって、まぁこれつけないとパフォーマンス上使いものにならんケースが多いようです。

ただ、このタイプの rwlock を使うと、 thread 1 が calcSomething の reader lock を取る→ thread 2 が setVal の writer lock を待ちはじめる→ thread 1 が getVal の reader lock を取ろうとするが、 writer が待ってるので取れない、という形のデッドロックになります。

教訓としては必要がなければ rwlock をそもそも使わない、ってのがあるかと思います。あとまぁ普通に reader lock だろうと nest させてると良くないなっていう。普通のロックに変えれませんし。

あと

condition variable のダメケースを作ろうかと思ったんですけど、あんま短くていい感じのが作れなかったので省略。一般に lock の外で cond_signal すると cond_wait で拾えないケースがある…ってやつが短く書けない。順序によっては普通に大丈夫なんですよね…

なにかあれば下記メールアドレスへ。
shinichiro.hamaji _at_ gmail.com
shinichiro.h