共有オブジェクトの設計

Question

質問

複数スレッドから読み書きを行う、共有オブジェクトの作成に悩んでいます。
オブジェクトは通信している一つ機器の状態を保持しています。

###条件
一つの機器の状態ですが、すべての状態において読み書きが同時にできないようしなくても
項目ごとに読み書きができなくなれば良いかなと考えております。
可読性が低くなりそうなら全体としても大丈夫です。

案

例として下記のようなDataManageクラスを使用しますが、
実際にはDataManageクラス内にその他情報もある想定です。
(DataManageは機器、State1／State2は機器の状態を項目分けしたものになります。)
項目を内部クラスとしていますが外でも大丈夫です。

　　1、プロパティで公開せず関数による項目ごとのアクセスにする。
    public class DataManage
    {
        public class State1
        {
            public int A { get; set; }
            public int B { get; set; }
        }

        public class State2
        {
            public int C { get; set; }
            public int D { get; set; }
        }

        private object syncState1 = new object();
        private object syncState2 = new object();
        private State1 state1 = new State1();
        private State2 state2 = new State2();

        public State1 GetState1()
        {
            var ret = new State1();
            lock (syncState1)
            {
                // ここでコピーなど
            }
            return ret;
        }
        public State2 GetState2()
        {
            var ret = new State2();
            lock (syncState2)
            {
                // ここでコピーなど
            }
            return ret;
        }
        public void SetState1(State1 state)
        {
            // ここでコピーなど
        }
        public void SetState2(State2 state)
        {
            // ここでコピーなど
        }
        /// <summary>
        /// 必要によって個々の状態にアクセスできるように。。。
        /// </summary>
        public int GetState1A()
        {
            lock (syncState1) return state1.A;
        }
        public void SetState1A( int val )
        {
            lock (syncState1) state1.A = val;
        }
    }

　　2、状態をさらにラップするクラスを設けプロパティでアクセスできるようにする。
    public class DataManage
    {

        public class State1Lap
        {
            public class State1
            {
                public int A { get; set; }
                public int B { get; set; }
            }

            private State1 state = new State1();
            private object syncState = new object();

            public int A
            {
                get { lock (syncState) return state.A; }
                set { lock (syncState) state.A = value; }
            }
            public int B
            {
                get { lock (syncState) return state.B; }
                set { lock (syncState) state.B = value; }
            }

            public State1 GetState()
            {
                var ret = new State1();
                lock (syncState)
                {
                    // ここでこぴーなど
                }
                return ret;
            }

            public void SetState(State1 state)
            {
                // ここでこぴーなど
            }

        }

        public class State2Lap
        {
            public class State2
            {
                public int C { get; set; }
                public int D { get; set; }
            }

            private State2 state = new State2();
            private object syncState = new object();

            public int C
            {
                get { lock (syncState) return state.C; }
                set { lock (syncState) state.C = value; }
            }
            public int D
            {
                get { lock (syncState) return state.D; }
                set { lock (syncState) state.D = value; }
            }

            public State2 GetState()
            {
                var ret = new State2();
                lock (syncState)
                {
                    // ここでこぴーなど
                }
                return ret;
            }

            public void SetState( State2 state )
            {
                // ここでこぴーなど
            }
        }
        private State1Lap state1 = new State1Lap();
        private State2Lap state2 = new State2Lap();

        public State1Lap State1 => state1;
        public State2Lap State2 => state2;
    }

　　3、そもそも項目ごとに分けずすべてを並列に管理する。
    public class DataManage
    {
        private object syncState = new object();
        private int a { get; set; }
        private int b { get; set; }

        private int c { get; set; }
        private int d { get; set; }

        public int A
        {
            get { lock (syncState) return a;  }
            set { lock (syncState)  a = value;  }
        }
        public int B
        {
            get { lock (syncState) return b; }
            set { lock (syncState) b = value; }
        }

        public int C
        {
            get { lock (syncState) return c; }
            set { lock (syncState) c = value; }
        }
        public int D
        {
            get { lock (syncState) return d; }
            set { lock (syncState) d = value; }
        }

        public DataManage Clone()
        {
            var datamanage = new DataManage();
            lock (syncState)
            {
                // コピーなど
            }
            return datamanage;
        } 
    }

　　4、そもそもの項目オブジェクトを保証できるようにしておく
    public class DataManage
    {
        public class State1
        {
            private object syncState = new object();
            private int a { get; set; }
            private int b { get; set; }

            public int A
            {
                get { lock (syncState) return a; }
                set { lock (syncState) a = value; }
            }
            public int B
            {
                get { lock (syncState) return b; }
                set { lock (syncState) b = value; }
            }

            public State1 Clone()
            {
                var ret = new State1();
                lock (syncState)
                {
                    // ここでこぴーなど
                }
                return ret;
            }

            public void Copy(State1 state)
            {
                // ここでこぴーなど
            }
        }

        public class State2
        {
            private object syncState = new object();
            private int c { get; set; }
            private int d { get; set; }

            public int C
            {
                get { lock (syncState) return c; }
                set { lock (syncState) c = value; }
            }
            public int D
            {
                get { lock (syncState) return d; }
                set { lock (syncState) d = value; }
            }

            public State2 Clone()
            {
                var ret = new State2();
                lock (syncState)
                {
                    // ここでこぴーなど
                }
                return ret;
            }

            public void Copy( State2 state )
            {
                // ここでこぴーなど
            }
        }
        private State1 state1 = new State1();
        private State2 state2 = new State2();

        public State1 State1 => state1;
        public State2 State2 => state2;
    }

考え

1、値の保証はできるが、プロパティとして個々の変数にアクセスすることができず、
個々の値が欲しいのに、状態全体を取得しないといけない。

2、一番ましなように思えます。

3、保証はされているが、State1／State2としてかたまりで取得できない。

4、値を保証するといった観点では目標を達成しているが、オブジェクトをDataManageからローカルにコピーして使用する場合一つのスレッドからしか読み書きが行われないので、保証する必要がないのでなしかなと。

###　聞きたいこと
複数のスレッドから書き込み／読み込みが行われる想定の値を保持したクラスをどのように作成したらよいかです。

他にもいい方法や指摘などがあればよろしくお願いいたします。

Comments

[退会済みユーザー]

単なるアンケートになっていて、どういう問題があって、どうなれば解決かが示されていないので回答出来ないです。特に大きな問題がなければ、とりあえず一番ましと思われる方法でやってみればよいのではないでしょうか？
Interlocked クラスというものもあるので、一応紹介しておきます。
https://docs.microsoft.com/ja-jp/dotnet/api/system.threading.interlocked

[neconekocat]

提示されてるクラスが色々と問題あってどう答えればいいか分からないんですよね
1はそもそもロックが意味をなしていないしSetStateはインスタンスの上書きでなくディープコピーをやりたがっているように見える
あくまでサンプルを見ただけで言えば3が一番マトモに見えますよ

[bell_21]

radianさん
指摘ありがとうございます。確かにアンケートになっていますね、、
解決方法というよりは、どのようにクラスの設計をしたら良いのかを聞きたかった次第です。

[bell_21]

neconekocatさん、ありがとうございます。
経験が浅いので問題が自分には上げられないのですが、よろしければ教えていただけないでしょうか。
１に関しては確かにディープコピーをする想定でした。
設定するほうにもロックが必要な感じですかね、、

[neconekocat]

そもそもロックはどこからどこまで必要かを勘違いしているような気がします。
ロックする範囲は「値を取得(設定する)必要がある処理の開始時から終了時まで」です。
例えば、通信機器が通信中かどうかの状態が格納されているとします。
ここで、「通信中ではないならば通信を開始し、状態を通信中にする」という処理を実行する場合、最初から最後までロックを続けなければいけません。値を確認した後にロックを解除した場合、通信開始前に他スレッドが値を書き換えてしまう可能性がありますからね。

[bell_21]

確かにロックをかける範囲の認識が間違っていと思います。
「読み込んでから状態によって書き込む」みたいな動作を複数のスレッドが行う場合、セマフォやミューテックス(ロックならロックするオブジェクトを読み取り専用で公開？)みたいにするのでしょうか。

[neconekocat]

既に回答者が出ていますのでこれで終わりますが、要件によるとしか言えません。
その例の場合であればプロパティ毎にロック用のものを個別に用意します。
またState1のようにデータを構造体として纏めたい場合はイミュータブルなクラスにし、値書き換え=別インスタンスをセットするようにします。構造体として纏める意味がないなら個別のプロパティとして実装するほうが設計としてはキレイです。(3が一番マトモに見えると書いたのはそれが理由です)

[退会済みユーザー]

この質問は何をもってクローズするつもりなのでしょう？
再設計するという結論になったのであれば、質問内容自体がほぼ無意味なものになりますので、一旦クローズし、再設計後に具体的な問題が出てきたら新たに質問してください。
明確な答えがない質問をダラダラと引き延ばすつもりなら低評価せざるを得ません。

[bell_21]

すみません、クローズしわすれました。
仕様を再確認し疑問があれば再度ここで質問しようと思います。

Answer 1

あくまで例ですがlock用のobjectを用意したい場合だとそれっぽいクラスでラップしたりします。

public class LockManager<T>
{
    public object LockObject { get; } = new object();
    public T Value { get; set; } = default(T);
}

public class DataManage
{
    public LockManager<int> A = new LockManager<int>();
    public LockManager<int> B = new LockManager<int>();
}

private static readonly DataManage m_Data = new DataManage();

static void Test()
{
    // 値の更新
    lock (m_Data.A.LockObject)
        m_Data.A.Value = 100;

    // 値の参照
    lock (m_Data.A.LockObject)
        Console.WriteLine($"A = {m_Data.A.Value});
}

ただしロック対象を細分化すると、当然デッドロックさせてしまう可能性も高まります。

static void deadlock()
{
    Task.Run(() =>
    {
        lock(m_Data.A.LockObject)
        {
            Thread.Sleep(200);
            lock (m_Data.B.LockObject) // ←ここと
                Console.WriteLine($"A = {m_Data.A.Value}, B = {m_Data.B.Value}");
        }
    });

    Thread.Sleep(100);

    lock (m_Data.B.LockObject)
        lock (m_Data.A.LockObject) // ←ここで死ぬ
            Console.WriteLine($"A = {m_Data.A.Value}, B = {m_Data.B.Value}");
}

・同時にアクセスする可能性がある変数は1つのクラスにしてクラス単位でアクセスする
・ReaderWriterLockSlimクラスのようにデッドロックを防ぎやすいクラスを使う
等、考慮すべきことが非常に多いため一概にコレがいいというやり方はありません。
まずは要件を分析し、必要な仕様の洗い出しをしてからクラス設計を考えて下さい。

Comments

[bell_21]

まとめた構造にしたい場合はクラス単位でのアクセスにしたほうが確かにいいかもしれないです。
ReaderWriterLockSlimについては、少し調べさせていただきます。
やはり、あいまいな状態で設計を行うのがまず間違いですね。。

Answer 2

こういう質問が出てくる時点で、ロックによって何を実現したいか・保証したいかが仕様として明確になっていないのではないですか？
例えば、1でGetState1する時、State1をコピーして返す感じに見えますが、
【GetState1呼び出し直後のState1】：A=100 B=200
【コピー中に別スレッドからB=300に書き換えが発生】：A=100 B=300
【実際に返されたState1】A=100 B=300
という結果が戻ってきて問題ないのかどうか、という事です。GetState1呼び出し直後のState1の状態を保証したいのであれば、その間にState1に書き込めたらまずいから、コピーが終わるまで書き込みをロックしないといけないですよね。
単純なint値の読み書きだけなら、そもそもロック不要な事が多いです。
何故ロックする必要があるのか、いつからいつまでロックしないといけないのか、を仕様として整理し直せば自ずとクラス設計も決まってくるでしょう。

Comments

[bell_21]

確かにご指摘の通り、仕様があやふやな状態でプログラムを作ろうとしていました、、
仕様を整理するのが先ですね。
すみませんが、１のコードですが、SetState1にも同様なロックが抜けておりました。
普通という聞き方はよくはないと思いますが、普通データの操作中をロックする場合、データとは関係ないところでロック用のオブジェクトを定義／参照して行う(Lock使用)。または、データ内部に設定変更中(ManualResetEvent等)などを設け行う。
などがある？かと思いますがどのようにするのでしょうか。
教えていただけると幸いです。

[退会済みユーザー]

ManualResetEventは複数のスレッド間で、実行タイミングの調整に使うものです。単純な排他用途なら普通にlockでいいです。同期オブジェクトに関する記事は、検索したら詳しい記事がいくらでも出てくると思うので自分で調べてみてください。

[bell_21]

ありがとうございます。
知識不足を再認識しました。