スレッドの仕組みを使い、キャラクター同士で戦わせるプログラムでHPが0になっても処理が続いてしまう。

Question

前提・実現したいこと

スレッドを使って、キャラクター同士で戦闘を行い、攻撃を仕掛ける度に攻撃を受けた側のHPをー１する。
どちらかのキャラクターのHPが0になったらその時点で処理をやめる。
HPが0になったキャラクターは攻撃してはならず、攻撃側もHP0のキャラを攻撃できない。

発生している問題・エラーメッセージ

どちらかのHPが０になっても、1回だけ死んだはずのキャラが攻撃してしまう。

実際の実行結果

Rock attacked Jay , Rock:10 , Jay:9
Rock attacked Jay , Rock:10 , Jay:8
Jay attacked Rock , Jay:8 , Rock:9
Rock attacked Jay , Rock:9 , Jay:7
Jay attacked Rock , Jay:7 , Rock:8
Rock attacked Jay , Rock:7 , Jay:6
Jay attacked Rock , Jay:6 , Rock:7
Jay attacked Rock , Jay:6 , Rock:6
Rock attacked Jay , Rock:6 , Jay:5
Rock attacked Jay , Rock:6 , Jay:4
Rock attacked Jay , Rock:6 , Jay:3
Jay attacked Rock , Jay:3 , Rock:5
Rock attacked Jay , Rock:5 , Jay:2
Jay attacked Rock , Jay:2 , Rock:4
Rock attacked Jay , Rock:4 , Jay:1
Jay attacked Rock , Jay:1 , Rock:3
Jay attacked Rock , Jay:1 , Rock:2
Rock attacked Jay , Rock:2 , Jay:0
Jay attacked Rock , Jay:0 , Rock:1
↑でJayは死んでいるはずなのに攻撃している。
Jaylost.

該当のソースコード

class Fight{
	public static void main(String args[]) {
		RPGCharacter c1 = null,c2 = null;
		c1 = new RPGCharacter("Jay",10);
		c2 = new RPGCharacter("Rock",10);
		c1.setTarget(c2);
		c2.setTarget(c1);
		c1.start();
		c2.start();
	}
}
class RPGCharacter extends Thread{
	String name;
	int hp;
	RPGCharacter target;
	Object lock = new Object();

	RPGCharacter(String name,int hp){
		this.name = name; this.hp = hp;
	}
	void setTarget(RPGCharacter target) {this.target = target;}
	RPGCharacter getTarget(){synchronized(lock) {return target;}}
	void attack() {
		synchronized(lock) {
			while(hp>0&&target.hp>0) {
				try {Thread.sleep((int)(10*Math.random()));}
				catch(InterruptedException ie) {}

				if(target.hp>0)target.hp--;
				System.out.println(name + " attacked " + target.name + " , " + name + ":" + hp + " , " + target.name + ":" + target.hp); 
			}
			if(hp<=0) {
				System.out.println(name + "lost.");
			}
		}
	}
	public void run() {
		attack();
	}
}

試したこと

synchronizedの書き方をいろいろ試したのですがどうもうまくいきません！c1とc2、同時に処理が行われているから、HPが0になった判定while(hp>0&&target.hp>0)を受ける前に攻撃の処理をしてしまうっていうことは何となくわかるのですが、、
同期処理についての理解が足りないのでしょうか？

補足情報（FW/ツールのバージョンなど）

Java初心者なので、質問内容等で不快にさせてしまったら申し訳ありません！

Answer 1

ロックによる同期は、複数のスレッドが同じモノをロック・アンロックすることで機能するのであって、それぞれのスレッドがそれぞれのモノをロック・アンロックしていては、同期になりません。

死んでいるのに攻撃してしまうのは、 while ループがそう書かれているからでしょう。
while ループは
・hpチェック
・スリープ
・攻撃
を繰り返します。
スリープ中に攻撃を受けて hp が 0 にされても、攻撃してからでなければチェックは行われません。

もう少し言いますと、それぞれのキャラクターはそれぞれ自分の hp を管理すべきで、攻撃したからと言って相手の hp を減らす処理を攻撃した側が行うのは変ではないでしょうか。
例えば「攻撃を受けた場合に相手から呼ばれる」wasAttacked メソッドを作り、 c1 が c2 を攻撃したなら c1 が c2 の wasAttacked を呼びます。
wasAttacked は自身の hp を減らし、 0 になったら自分に interrupt を掛けます。
attacked メソッドは sleep が interrupted 例外を発したら while ループを抜けるようにすれば、 hp 0 で即スレッドは終了するでしょう。
相手が倒れたかを wasAttacked の戻り値で分かるようにすれば、攻撃した側も戦闘の終了が分かります。
ですので while の条件で hp をチェックする必要もなく、 !interrupted() だけで良いです。
そしてこの場合、ロックによる同期は必要ありません。

---

「ロックによる同期は必要ありません」と書きましたが、攻撃した後の表示で問題が有りそうですので、ロックを入れました。

攻撃側が相手の wasAttacked を呼んでから hp を表示するまでの間に相手がこちらの wasAttacked を呼んでこちらの hp を減らし、その後こちらの表示が行われると、相手が攻撃した表示がされる前に hp が減っている状態になるからです。
つまり、相手の hp を変更してから(相手の hp を減らしたことを)表示するまでの間で(同時に表示している)自分の hp が変更されてしまうのを防ぐ為にロックします。

デッドロックが発生するため、 lock オブジェクトを static にしました。
lock の取り合いで負けた場合に倒される可能性があるため、ロック取得後に割り込みチェックを入れました。

package teratail_java.q373120;

public class Fight {
  public static void main(String args[]) {
    RPGCharacter c1 = new RPGCharacter("Jay", 10);
    RPGCharacter c2 = new RPGCharacter("Rock", 10);
    c1.setTarget(c2);
    c2.setTarget(c1);
    c1.start();
    c2.start();
  }
}

class RPGCharacter extends Thread {
  private static Object[] lock = new Object[0];

  private String name;
  private int hp;
  private RPGCharacter target;

  RPGCharacter(String name, int hp) {
    this.name = name;
    this.hp = hp;
  }

  void setTarget(RPGCharacter target) {
    this.target = target;
  }

  void attack() {
      while(!interrupted()) {
        try {
          Thread.sleep((long) (10 * Math.random()));
        } catch(InterruptedException ie) {
          break; //倒された
        }

        synchronized(lock) {
          if(interrupted()) break; //倒された
          boolean down = target.wasAttacked();
          System.out.println(name + " attacked " + target.name + " , " + this + " , " + target);
          if(down) break; //倒した
        }
      }

      if(hp <= 0) {
        System.out.println(name + " lost.");
      }
  }

  @Override
  public void run() {
    attack();
  }

  boolean wasAttacked() {
    if(--hp > 0) return false; //まだ倒れない
    interrupt();
    return true; //倒れた
  }

  @Override
  public String toString() {
    return name + ":" + hp;
  }
}

Jay attacked Rock , Jay:10 , Rock:9
Rock attacked Jay , Rock:9 , Jay:9
Jay attacked Rock , Jay:9 , Rock:8
Jay attacked Rock , Jay:9 , Rock:7
Rock attacked Jay , Rock:7 , Jay:8
Jay attacked Rock , Jay:8 , Rock:6
Rock attacked Jay , Rock:6 , Jay:7
Jay attacked Rock , Jay:7 , Rock:5
Jay attacked Rock , Jay:7 , Rock:4
Rock attacked Jay , Rock:4 , Jay:6
Jay attacked Rock , Jay:6 , Rock:3
Rock attacked Jay , Rock:3 , Jay:5
Rock attacked Jay , Rock:3 , Jay:4
Jay attacked Rock , Jay:4 , Rock:2
Rock attacked Jay , Rock:2 , Jay:3
Jay attacked Rock , Jay:3 , Rock:1
Rock attacked Jay , Rock:1 , Jay:2
Rock attacked Jay , Rock:1 , Jay:1
Rock attacked Jay , Rock:1 , Jay:0
Jay lost.

Comments

[remontaruto8325]

とても分かりやすい解説ありがとうございました！無事望み通りの実行結果が出せました！