persistent_callback_の呼び出し順が正しくなくなることがある。

[umegane]

• Issue #61 と同様の問題が、persistent_callback_の呼び出しでも存在する。
  • compare_exchange_strong のチェック後mutexでロックを取り、さいどcompare後、persistent_callback_を呼べば良い

to_be_epoch = upper_limit;
old_epoch_id = epoch_id_informed_.load();
while (true) {
    if (old_epoch_id >= to_be_epoch) {
        break;
    }
    if (epoch_id_informed_.compare_exchange_strong(old_epoch_id, to_be_epoch)) {
        if (persistent_callback_) {
            persistent_callback_(to_be_epoch);
        }
        break;
    }
}

[umegane]

次のように対応しました。

to_be_epoch = upper_limit;
old_epoch_id = epoch_id_informed_.load();
while (true) {
    if (old_epoch_id >= to_be_epoch) {
        break;
    }
    if (epoch_id_informed_.compare_exchange_strong(old_epoch_id, to_be_epoch)) {
        std::lock_guard<std::mutex> lock(mtx_epoch_persistent_callback_);
        if (to_be_epoch < epoch_id_informed_.load()) {
            break;
        }
        if (persistent_callback_) {
            persistent_callback_(to_be_epoch);
        }
        break;
    }
}

[umegane]

@t-horikawa Issue-63 のこの議論から、この修正は問題ありなのでリオープンします。

1. 修正前は、persistent_callback_の呼び出し順がEpoch IDの順にならず前後する可能性があうという問題があります。
2. 修正後は、Epoch IDの飛び越しはなくなりますが、スキップされるEpoch IDが発生する可能性があります。

persistent_callback_の呼び出しをスキップしてはいけないということなので、2.の修正はNGです。1.が許容可能であれば、 1.に戻せばよいです。1.も許容できない場合は、Queueを使うなどして順序を保証するようにしないといけません。

[umegane]

@t-horikawa すいません、このコードでも問題ないような気がしてきました。 よく考えるのでしばらくの間、上のメンションは無視してください。

[t-horikawa]

persistent_callbackで「スキップされるEpoch IDが発生する」のがNGかどうかはshirakami動作上、問題ありなのかどうか、要確認と思います。 persistent_callbackで明らかに問題だっだのは、switch_epochによりepochがどんどん進んでいるのに、persistent_callbackが一切callされないことでした（limestone開発の極々初期にはlog_channelが使われないとそうなってしまった）。指摘されている問題だと、epoch IDがスキップされても、次のepoch IDでpersistent_callbackされるので、一切callされないという状況にはなりません。

[umegane]

どちらのコードでも、スキップされる可能性がありますね。スキップの是非については、shirakmiの動作上の問題なので shirakamiの仕様をか確認します。

[umegane]

以下の方針で対応します。

• Limestoneの仕様は現状通りとする。
• 現状の仕様をドキュメント化する
• Shirkamiが現状の仕様で問題ないかの確認、及び必要に応じて修正するという内容のIssueを別途作成する

すべて終了したらこのIssueはクローズします。