From be0d6f88b8c258bc2036d08ed8aa20a45c85d2cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Garci=CC=81a=20Vega?= Date: Sat, 14 May 2016 10:29:02 +0200 Subject: [PATCH 1/3] Monitoring refactor WIP --- lib/battleship/game.ex | 60 ++++++++++++++++++------ lib/battleship/game/board.ex | 14 ++++-- lib/battleship/game/event.ex | 1 + lib/battleship/game/event_handler.ex | 8 +++- web/channels/game_channel.ex | 16 +++++-- web/static/js/actions/game.js | 5 ++ web/static/js/reducers/game.js | 3 ++ web/static/js/routes/index.js | 2 + web/static/js/views/errors/game_error.js | 31 ++++++++++++ web/static/js/views/errors/not_found.js | 4 +- 10 files changed, 119 insertions(+), 25 deletions(-) create mode 100644 web/static/js/views/errors/game_error.js diff --git a/lib/battleship/game.ex b/lib/battleship/game.ex index 93c371a..fb3df6e 100644 --- a/lib/battleship/game.ex +++ b/lib/battleship/game.ex @@ -25,14 +25,34 @@ defmodule Battleship.Game do def join(id, player_id, pid), do: try_call(id, {:join, player_id, pid}) + @doc """ + Returns the game's state + """ def get_data(id), do: try_call(id, :get_data) + + @doc """ + Returns the game's state for a given player. This means it will + hide ships positions from the opponent's board. + """ def get_data(id, player_id), do: try_call(id, {:get_data, player_id}) + @doc """ + Adds new chat message to the game's state + """ def add_message(id, player_id, text), do: try_call(id, {:add_message, player_id, text}) + @doc """ + Fires a shot into the opponent's board for the given coordinates + """ def player_shot(id, player_id, x: x, y: y), do: try_call(id, {:player_shot, player_id, x: x, y: y}) + + @doc """ + Called when a player leaves the game + """ def player_left(id, player_id), do: try_call(id, {:player_left, player_id}) + def stop_game(id), do: try_call(id, :stop) + # SERVER def init(id) do @@ -51,9 +71,10 @@ defmodule Battleship.Game do {:reply, {:ok, self}, game} true -> Process.monitor(pid) - Process.flag(:trap_exit, true) + # Process.flag(:trap_exit, true) - create_board(player_id) + {:ok, board_pid} = create_board(player_id) + Process.monitor(board_pid) game = game |> add_player(player_id) @@ -102,6 +123,10 @@ defmodule Battleship.Game do {:reply, {:ok, game}, game} end + def handle_call(:stop, _from, game) do + stop(game) + end + def get_opponents_id(%Game{attacker: player_id, defender: nil}, player_id), do: nil def get_opponents_id(%Game{attacker: player_id, defender: defender}, player_id), do: defender def get_opponents_id(%Game{attacker: attacker, defender: player_id}, player_id), do: attacker @@ -109,23 +134,26 @@ defmodule Battleship.Game do @doc """ Handles exit messages from linked game channels processes, destroying boards and sopping the game process. - - {:DOWN, _ref, :process, _pid, _reason} - - {:EXIT, _pid, {:shutdown, :closed}} + + - {:DOWN, _ref, :process, _pid, _reason} + - {:EXIT, _pid, {:shutdown, :closed}} """ - def handle_info(message, game) do - Logger.debug "Handling message #{inspect message} in Game server" + def handle_info({:DOWN, _ref, :process, _pid, _reason}, game) do + Logger.debug "Handling :DOWM in Game server" + Battleship.Game.Event.game_stopped(game.id) stop(game) end + # def handle_info({:EXIT, _pid, {:shutdown, :closed}}, game) do + # Logger.debug "Handling :EXIT message in Game server" + # + # stop(game) + # end - @doc """ - Creates a new Board for a given Player - """ + # Creates a new Board for a given Player defp create_board(player_id), do: Board.create(player_id) - @doc """ - Generates global reference - """ + # Generates global reference defp ref(id), do: {:global, {:game, id}} defp add_player(%__MODULE__{attacker: nil} = game, player_id), do: %{game | attacker: player_id} @@ -133,10 +161,9 @@ defmodule Battleship.Game do defp add_channel(game, pid), do: %{game | channels: [pid | game.channels]} - defp destroy_board(nil), do: :ok - defp destroy_board(player_id), do: Board.destroy(player_id) - defp stop(game) do + Logger.debug "Stopping Game #{game.id}" + for player <- [game.attacker, game.defender], do: destroy_board(player) Battleship.Game.Event.game_over @@ -144,6 +171,9 @@ defmodule Battleship.Game do {:stop, :normal, game} end + defp destroy_board(nil), do: :ok + defp destroy_board(player_id), do: Board.destroy(player_id) + defp udpate_turns(game, player_id, x: x, y: y, result: result) do %{game | turns: [%{player_id: player_id, x: x, y: y, result: result} | game.turns]} end diff --git a/lib/battleship/game/board.ex b/lib/battleship/game/board.ex index 859086c..c1dd762 100644 --- a/lib/battleship/game/board.ex +++ b/lib/battleship/game/board.ex @@ -37,9 +37,14 @@ defmodule Battleship.Game.Board do Destroys an existing Player board """ def destroy(player_id) do - Logger.debug "Stopping board for player #{player_id}" - - Agent.stop(ref(player_id), :normal, :infinity) + case GenServer.whereis(ref(player_id)) do + nil -> + Logger.debug "Attempt to destroy unesxisting Board for player #{player_id}" + pid -> + Logger.debug "Stopping board for player #{player_id}" + + Agent.stop(pid, :normal, :infinity) + end end @doc """ @@ -121,6 +126,7 @@ defmodule Battleship.Game.Board do {:ok, result} end + # Generates global reference name for the board process defp ref(player_id), do: {:global, {:board, player_id}} # Checks if a similar ship has been already placed @@ -167,6 +173,7 @@ defmodule Battleship.Game.Board do |> Enum.reduce(%{}, fn item, acc -> Map.put(acc, item, @grid_value_water) end) end + # Builds cells for a given row defp build_rows(y, rows) do row = 0..@size - 1 |> Enum.reduce(rows, fn x, col -> [Enum.join([y, x], "") | col] end) @@ -174,6 +181,7 @@ defmodule Battleship.Game.Board do [row | rows] end + # Returns shot result depending on the cell's current value defp shot_result(current_value) when current_value == @grid_value_ship, do: @grid_value_ship_hit defp shot_result(current_value) when current_value == @grid_value_ship_hit, do: @grid_value_ship_hit defp shot_result(_current_value), do: @grid_value_water_hit diff --git a/lib/battleship/game/event.ex b/lib/battleship/game/event.ex index 83afa7a..84d1b26 100644 --- a/lib/battleship/game/event.ex +++ b/lib/battleship/game/event.ex @@ -14,5 +14,6 @@ defmodule Battleship.Game.Event do def game_created, do: GenEvent.notify(__MODULE__, :game_created) def player_joined, do: GenEvent.notify(__MODULE__, :player_joined) def game_over, do: GenEvent.notify(__MODULE__, :game_over) + def game_stopped(game_id), do: GenEvent.notify(__MODULE__, {:game_stopped, game_id}) def player_shot, do: GenEvent.notify(__MODULE__, :player_shot) end diff --git a/lib/battleship/game/event_handler.ex b/lib/battleship/game/event_handler.ex index 2358562..8264870 100644 --- a/lib/battleship/game/event_handler.ex +++ b/lib/battleship/game/event_handler.ex @@ -1,12 +1,16 @@ defmodule Battleship.Game.EventHandler do use GenEvent - alias Battleship.LobbyChannel + alias Battleship.{LobbyChannel, GameChannel} + def handle_event({:game_stopped, game_id}, state) do + GameChannel.broadcast_stop(game_id) + + {:ok, state} + end def handle_event(:game_created, state), do: broadcast_update(state) def handle_event(:player_joined, state), do: broadcast_update(state) def handle_event(:game_over, state), do: broadcast_update(state) def handle_event(:player_shot, state), do: broadcast_update(state) - def handle_event(_, state), do: {:ok, state} defp broadcast_update(state) do diff --git a/web/channels/game_channel.ex b/web/channels/game_channel.ex index 85b928a..0eecd88 100644 --- a/web/channels/game_channel.ex +++ b/web/channels/game_channel.ex @@ -13,8 +13,8 @@ defmodule Battleship.GameChannel do player_id = socket.assigns.player_id case Game.join(game_id, player_id, socket.channel_pid) do - {:ok, pid} -> - Process.link(pid) + {:ok, _pid} -> + Process.flag(:trap_exit, true) {:ok, assign(socket, :game_id, game_id)} {:error, reason} -> @@ -107,13 +107,23 @@ defmodule Battleship.GameChannel do case Game.player_left(game_id, player_id) do {:ok, game} -> + broadcast(socket, "game:over", %{game: %{game | channels: nil}}) broadcast(socket, "game:player_left", %{player_id: player_id}) - :ok + Game.stop_game(game_id) + :ok _ -> :ok end end + + def handle_info(_message, socket) do + {:noreply, socket} + end + + def broadcast_stop(game_id) do + Battleship.Endpoint.broadcast("game:#{game_id}", "game:stopped", %{}) + end end diff --git a/web/static/js/actions/game.js b/web/static/js/actions/game.js index ffc72d6..b686dbe 100644 --- a/web/static/js/actions/game.js +++ b/web/static/js/actions/game.js @@ -58,6 +58,11 @@ export function joinGame(socket, playerId, gameId) { playerId: payload.player_id, }); }); + + channel.on(`game:stopped`, (payload) => { + dispatch(resetGame()); + dispatch(push('/game_error')); + }); }; } diff --git a/web/static/js/reducers/game.js b/web/static/js/reducers/game.js index 816b9d3..32f812e 100644 --- a/web/static/js/reducers/game.js +++ b/web/static/js/reducers/game.js @@ -105,6 +105,9 @@ export default function reducer(state = initialState, action = {}) { return { ...state, error: action.error }; case Constants.GAME_RESET: + let { gameChannel } = state; + if (gameChannel != null) gameChannel.leave(); + return { ...initialState }; default: diff --git a/web/static/js/routes/index.js b/web/static/js/routes/index.js index db2191a..e86ffb8 100644 --- a/web/static/js/routes/index.js +++ b/web/static/js/routes/index.js @@ -4,6 +4,7 @@ import MainLayout from '../layouts/main'; import HomeIndexView from '../views/home'; import GameShowView from '../views/game/show'; import NotFoundView from '../views/errors/not_found'; +import GameErrorView from '../views/errors/game_error'; export default function configRoutes(store) { return ( @@ -11,6 +12,7 @@ export default function configRoutes(store) { + ); diff --git a/web/static/js/views/errors/game_error.js b/web/static/js/views/errors/game_error.js new file mode 100644 index 0000000..d28699e --- /dev/null +++ b/web/static/js/views/errors/game_error.js @@ -0,0 +1,31 @@ +import React, { PropTypes } from 'react'; +import { Link } from 'react-router'; +import { connect } from 'react-redux'; +import NewGameButton from '../../components/game/new_game_button'; +import Logo from '../../components/common/logo'; +import { setDocumentTitle } from '../../utils'; + +class GameErrorView extends React.Component { + componentDidMount() { + setDocumentTitle('Blow me down, game error!'); + } + + render() { + const { lobbyChannel, dispatch } = this.props; + + return ( +
+ +

Blow me down, game error!

+ Start new battle, arr! +

Back to home

+
+ ); + } +} + +const mapStateToProps = (state) => ( + { ...state.session, ...state.game } +); + +export default connect(mapStateToProps)(GameErrorView); diff --git a/web/static/js/views/errors/not_found.js b/web/static/js/views/errors/not_found.js index 3bd83dc..692c7e6 100644 --- a/web/static/js/views/errors/not_found.js +++ b/web/static/js/views/errors/not_found.js @@ -1,4 +1,4 @@ -import React, {PropTypes} from 'react'; +import React, { PropTypes } from 'react'; import { Link } from 'react-router'; import { connect } from 'react-redux'; import NewGameButton from '../../components/game/new_game_button'; @@ -25,7 +25,7 @@ class NotFoundView extends React.Component { } const mapStateToProps = (state) => ( - state.session + { ...state.session, ...state.game } ); export default connect(mapStateToProps)(NotFoundView); From 5ae2b0912f52c3632e0a7dd33096adb9765ceb8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Garci=CC=81a=20Vega?= Date: Sat, 14 May 2016 12:10:59 +0200 Subject: [PATCH 2/3] Terminate games from supervisor, and clean boards while terminating game --- lib/battleship/game.ex | 36 ++++++++++++++----------------- lib/battleship/game/supervisor.ex | 10 +++++++++ web/channels/game_channel.ex | 28 +++++++++++++----------- web/channels/lobby_channel.ex | 3 +++ 4 files changed, 44 insertions(+), 33 deletions(-) diff --git a/lib/battleship/game.ex b/lib/battleship/game.ex index fb3df6e..e73f346 100644 --- a/lib/battleship/game.ex +++ b/lib/battleship/game.ex @@ -6,6 +6,7 @@ defmodule Battleship.Game do require Logger alias Battleship.{Game} alias Battleship.Game.Board + alias Battleship.Game.Supervisor, as: GameSupervisor defstruct [ id: nil, @@ -51,8 +52,6 @@ defmodule Battleship.Game do """ def player_left(id, player_id), do: try_call(id, {:player_left, player_id}) - def stop_game(id), do: try_call(id, :stop) - # SERVER def init(id) do @@ -71,7 +70,7 @@ defmodule Battleship.Game do {:reply, {:ok, self}, game} true -> Process.monitor(pid) - # Process.flag(:trap_exit, true) + Process.flag(:trap_exit, true) {:ok, board_pid} = create_board(player_id) Process.monitor(board_pid) @@ -123,10 +122,6 @@ defmodule Battleship.Game do {:reply, {:ok, game}, game} end - def handle_call(:stop, _from, game) do - stop(game) - end - def get_opponents_id(%Game{attacker: player_id, defender: nil}, player_id), do: nil def get_opponents_id(%Game{attacker: player_id, defender: defender}, player_id), do: defender def get_opponents_id(%Game{attacker: attacker, defender: player_id}, player_id), do: attacker @@ -138,11 +133,12 @@ defmodule Battleship.Game do - {:DOWN, _ref, :process, _pid, _reason} - {:EXIT, _pid, {:shutdown, :closed}} """ - def handle_info({:DOWN, _ref, :process, _pid, _reason}, game) do - Logger.debug "Handling :DOWM in Game server" + def handle_info({:DOWN, ref, :process, _pid, reason}, game) do + Logger.debug "Handling :DOWM in Game server with reason #{reason}" Battleship.Game.Event.game_stopped(game.id) - stop(game) + + {:stop, :normal, game} end # def handle_info({:EXIT, _pid, {:shutdown, :closed}}, game) do # Logger.debug "Handling :EXIT message in Game server" @@ -150,6 +146,16 @@ defmodule Battleship.Game do # stop(game) # end + def terminate(_reason, game) do + Logger.debug "Terminating Game process #{game.id}" + + for player <- [game.attacker, game.defender], do: destroy_board(player) + + Battleship.Game.Event.game_over + + :ok + end + # Creates a new Board for a given Player defp create_board(player_id), do: Board.create(player_id) @@ -161,16 +167,6 @@ defmodule Battleship.Game do defp add_channel(game, pid), do: %{game | channels: [pid | game.channels]} - defp stop(game) do - Logger.debug "Stopping Game #{game.id}" - - for player <- [game.attacker, game.defender], do: destroy_board(player) - - Battleship.Game.Event.game_over - - {:stop, :normal, game} - end - defp destroy_board(nil), do: :ok defp destroy_board(player_id), do: Board.destroy(player_id) diff --git a/lib/battleship/game/supervisor.ex b/lib/battleship/game/supervisor.ex index 68f610a..207e5b1 100644 --- a/lib/battleship/game/supervisor.ex +++ b/lib/battleship/game/supervisor.ex @@ -2,6 +2,8 @@ defmodule Battleship.Game.Supervisor do @moduledoc """ Game processes supervisor """ + require Logger + use Supervisor alias Battleship.{Game} @@ -29,6 +31,14 @@ defmodule Battleship.Game.Supervisor do |> Enum.map(&game_data/1) end + def stop_game(game_id) do + Logger.debug "Stopping game #{game_id} in supervisor" + + pid = GenServer.whereis({:global, {:game, game_id}}) + + Supervisor.terminate_child(__MODULE__, pid) + end + defp game_data({_id, pid, _type, _modules}) do pid |> GenServer.call(:get_data) diff --git a/web/channels/game_channel.ex b/web/channels/game_channel.ex index 0eecd88..168edcf 100644 --- a/web/channels/game_channel.ex +++ b/web/channels/game_channel.ex @@ -5,16 +5,17 @@ defmodule Battleship.GameChannel do use Phoenix.Channel alias Battleship.{Game, Ship} alias Battleship.Game.Board + alias Battleship.Game.Supervisor, as: GameSupervisor require Logger def join("game:" <> game_id, _message, socket) do - Logger.debug "Joining Game channel", game_id: game_id + Logger.debug "Joining Game channel #{game_id}", game_id: game_id player_id = socket.assigns.player_id case Game.join(game_id, player_id, socket.channel_pid) do - {:ok, _pid} -> - Process.flag(:trap_exit, true) + {:ok, pid} -> + Process.monitor(pid) {:ok, assign(socket, :game_id, game_id)} {:error, reason} -> @@ -23,7 +24,8 @@ defmodule Battleship.GameChannel do end def handle_in("game:joined", _message, socket) do - Logger.debug "Broadcasting player joined" + Logger.debug "Broadcasting player joined #{socket.assigns.game_id}" + player_id = socket.assigns.player_id board = Board.get_opponents_data(player_id) @@ -41,7 +43,7 @@ defmodule Battleship.GameChannel do end def handle_in("game:send_message", %{"text" => text}, socket) do - Logger.debug "Handling send_message on GameChannel" + Logger.debug "Handling send_message on GameChannel #{socket.assigns.game_id}" player_id = socket.assigns.player_id message = %{player_id: player_id, text: text} @@ -52,7 +54,7 @@ defmodule Battleship.GameChannel do end def handle_in("game:place_ship", %{"ship" => ship}, socket) do - Logger.debug "Handling place_ship on GameChannel" + Logger.debug "Handling place_ship on GameChannel #{socket.assigns.game_id}" player_id = socket.assigns.player_id game_id = socket.assigns.game_id @@ -78,7 +80,7 @@ defmodule Battleship.GameChannel do end def handle_in("game:shoot", %{"y" => y, "x" => x}, socket) do - Logger.debug "Handling shoot on GameChannel" + Logger.debug "Handling shoot on GameChannel #{socket.assigns.game_id}" player_id = socket.assigns.player_id game_id = socket.assigns.game_id @@ -100,7 +102,7 @@ defmodule Battleship.GameChannel do end def terminate(reason, socket) do - Logger.debug"Terminating GameChannel #{inspect reason}" + Logger.debug"Terminating GameChannel #{socket.assigns.game_id} #{inspect reason}" player_id = socket.assigns.player_id game_id = socket.assigns.game_id @@ -108,22 +110,22 @@ defmodule Battleship.GameChannel do case Game.player_left(game_id, player_id) do {:ok, game} -> + GameSupervisor.stop_game(game_id) + broadcast(socket, "game:over", %{game: %{game | channels: nil}}) broadcast(socket, "game:player_left", %{player_id: player_id}) - Game.stop_game(game_id) - :ok _ -> :ok end end - def handle_info(_message, socket) do - {:noreply, socket} - end + def handle_info(_, socket), do: {:noreply, socket} def broadcast_stop(game_id) do + Logger.debug "Broadcasting game:stopped from GameChannel #{game_id}" + Battleship.Endpoint.broadcast("game:#{game_id}", "game:stopped", %{}) end end diff --git a/web/channels/lobby_channel.ex b/web/channels/lobby_channel.ex index 4fd1f70..703188e 100644 --- a/web/channels/lobby_channel.ex +++ b/web/channels/lobby_channel.ex @@ -2,6 +2,7 @@ defmodule Battleship.LobbyChannel do @moduledoc """ Lobby channel """ + require Logger use Battleship.Web, :channel alias Battleship.Game.Supervisor, as: GameSupervisor @@ -22,6 +23,8 @@ defmodule Battleship.LobbyChannel do end def broadcast_current_games do + Logger.debug "Broadcasting current games from LobbyChannel" + Battleship.Endpoint.broadcast("lobby", "update_games", %{games: GameSupervisor.current_games}) end end From ce19dc40a9205e7119a2440ba8837253269046cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Garci=CC=81a=20Vega?= Date: Sun, 15 May 2016 08:51:12 +0200 Subject: [PATCH 3/3] Added test for covering boards exits --- lib/battleship/game.ex | 2 +- lib/battleship/game/board.ex | 2 +- test/lib/game_test.exs | 21 +++++++++++++++++---- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/battleship/game.ex b/lib/battleship/game.ex index e73f346..ed731bc 100644 --- a/lib/battleship/game.ex +++ b/lib/battleship/game.ex @@ -133,7 +133,7 @@ defmodule Battleship.Game do - {:DOWN, _ref, :process, _pid, _reason} - {:EXIT, _pid, {:shutdown, :closed}} """ - def handle_info({:DOWN, ref, :process, _pid, reason}, game) do + def handle_info({:DOWN, _ref, :process, _pid, reason}, game) do Logger.debug "Handling :DOWM in Game server with reason #{reason}" Battleship.Game.Event.game_stopped(game.id) diff --git a/lib/battleship/game/board.ex b/lib/battleship/game/board.ex index c1dd762..b98719b 100644 --- a/lib/battleship/game/board.ex +++ b/lib/battleship/game/board.ex @@ -42,7 +42,7 @@ defmodule Battleship.Game.Board do Logger.debug "Attempt to destroy unesxisting Board for player #{player_id}" pid -> Logger.debug "Stopping board for player #{player_id}" - + Agent.stop(pid, :normal, :infinity) end end diff --git a/test/lib/game_test.exs b/test/lib/game_test.exs index e4f4921..5f39197 100644 --- a/test/lib/game_test.exs +++ b/test/lib/game_test.exs @@ -33,19 +33,30 @@ defmodule Battleship.GameTest do assert %Board{player_id: ^defender_id} = Agent.get({:global, {:board, defender_id}}, &(&1)) end - test "closes game when player goes down", %{attacker_id: attacker_id} do + test "terminates game when player goes down", %{attacker_id: attacker_id} do {:ok, pid} = GameSupervisor.create_game("new-game") - ref = Process.monitor(pid) spawn fn -> Game.join("new-game", attacker_id, self) end - assert catch_exit(Agent.get({:global, {:board, 1}}, &(&1))) + assert catch_exit(Agent.get({:global, {:board, attacker_id}}, &(&1))) assert_receive {:DOWN, ^ref, :process, ^pid, :normal} end + test "terminates game when a board goes down", %{attacker_id: attacker_id} do + {:ok, pid} = GameSupervisor.create_game("new-game") + Process.monitor(pid) + Game.join("new-game", attacker_id, self) + + {:global, {:board, attacker_id}} + |> GenServer.whereis + |> Process.exit(:kill) + + assert_receive {:DOWN, _ref, :process, pid, :normal} + end + test "updates turns after a shot", %{id: id, attacker_id: attacker_id, defender_id: defender_id} do GameSupervisor.create_game(id) @@ -54,7 +65,9 @@ defmodule Battleship.GameTest do %Ship{x: 1, y: 0, size: 4, orientation: :vertical}, %Ship{x: 2, y: 0, size: 3, orientation: :vertical}, %Ship{x: 3, y: 0, size: 2, orientation: :vertical}, - %Ship{x: 4, y: 0, size: 1, orientation: :vertical} + %Ship{x: 4, y: 0, size: 2, orientation: :vertical}, + %Ship{x: 5, y: 0, size: 1, orientation: :vertical}, + %Ship{x: 6, y: 0, size: 1, orientation: :vertical} ] Game.join(id, attacker_id, self)