The Page That Never Came Back

Replying to a process that has died costs nothing and tells you nothing. GenServer.reply/2 puts a message in a mailbox that will never be read, hands you back :ok, and lets you carry on. That is correct Erlang, and it is almost always harmless.

It cost me a browser page every single time it happened.

The pool it happened in is unremarkable: one GenServer per provider that lends out pages, caps how many can be open at once, and closes the browser when nobody has wanted one for a while. It had two bugs in it that took me longer to spot than I would like, because they share a shape. In both of them, a message arrives about a world that has already moved on.

The pool

The first version is about as plain as a pool gets. A free list, a counter, a ceiling, and a queue for callers who arrive when everything is lent out:

def handle_call(:checkout, from, state) do
  state = state |> ensure_started() |> cancel_idle_timer()

  cond do
    state.free != [] ->
      [page | rest] = state.free
      {:reply, {:ok, page}, %{state | free: rest, in_use: state.in_use + 1}}

    state.in_use < state.max_pages ->
      case Page.open(state.provider) do
        {:ok, page} -> {:reply, {:ok, page}, %{state | in_use: state.in_use + 1}}
        {:error, error} -> {:reply, {:error, error}, state}
      end

    true ->
      {:noreply, %{state | waiting: :queue.in(from, state.waiting)}}
  end
end

That last branch is the interesting one. Returning {:noreply, ...} while keeping from in a queue is how you park a caller: it stays blocked in its GenServer.call until somebody replies on its behalf.

Somebody being the next checkin.

The caller that went away

def handle_cast({:checkin, page}, state) do
  case :queue.out(state.waiting) do
    {{:value, waiter}, rest} ->
      GenServer.reply(waiter, {:ok, page})
      {:noreply, %{state | waiting: rest}}

    {:empty, _rest} ->
      state = %{state | free: [page | state.free], in_use: state.in_use - 1}
      {:noreply, schedule_idle_timer(state)}
  end
end

Read the first branch as if the waiter were dead, and the whole bug is in those three lines. The page is handed straight to the waiter, so it is not put back on the free list and in_use is not decremented. Both of those are correct when somebody is actually there to receive it.

Nothing checks that anybody is.

A caller can be gone for perfectly boring reasons. It hit its own timeout, or the request that wanted a page was cancelled, or the process crashed while waiting. None of that reaches the pool, and the reply it sends into the void returns :ok like every other reply it has ever sent.

So the page is in nobody’s hands, absent from the free list, and still counted as in use. It is gone. Do that twice with max_pages: 2 and the pool is empty for good, every caller blocking for the full 60 second checkout timeout.

I ran the current tests against that old code to be sure I was not telling myself a story:

1) test a waiter that dies while queued does not shrink the pool
   Assertion with == failed
   code:  assert Session.free_count("test") == 1
   left:  0
   right: 1

Zero free pages where there should be one. The page went to a dead process.

Ask to be told

The fix is the boring one, which is usually the right one. Monitor the caller when you park it, and you get a message when it dies:

true ->
  {pid, _tag} = from
  ref = Process.monitor(pid)
  {:noreply, %{state | waiting: :queue.in({ref, from}, state.waiting)}}

Then take the dead one out of the queue when that message lands:

def handle_info({:DOWN, ref, :process, _pid, _reason}, state) do
  {:noreply, %{state | waiting: drop_waiter(state.waiting, ref)}}
end

That alone would mostly work, and mostly is doing a lot of work in that sentence. A DOWN is a message like any other, so it sits in the mailbox behind whatever arrived first. A checkin that got there ahead of it would still pop a corpse off the queue. So popping checks too, and keeps going until it finds someone alive:

defp pop_live_waiter(queue) do
  case :queue.out(queue) do
    {:empty, rest} ->
      {:empty, rest}

    {{:value, {ref, {pid, _tag}} = entry}, rest} ->
      if Process.alive?(pid) do
        {:ok, entry, rest}
      else
        Process.demonitor(ref, [:flush])
        pop_live_waiter(rest)
      end
  end
end

Two mechanisms for one problem looks redundant until you notice they cover different windows. The monitor cleans up the queue in the background. The check at the pop covers the gap between a process dying and its DOWN reaching the front of the mailbox.

The browser that went away

The second bug lived in a line I would have sworn was fine:

def handle_call(:shutdown, _from, state), do: {:reply, :ok, close_browser(state)}

close_browser/1 closes the browser and resets the counters. Under no load at all, that is correct. Under load it wrecks two things at once.

Queued waiters are simply left in the queue. Nobody is going to reply to them now, so they hang until their own 60 second timeout expires, which the test shows in the least ambiguous way available:

3) test shutdown rejects queued waiters instead of abandoning them
   ** (exit) exited in: Task.await(..., 1000)
       ** (EXIT) time out

The other half is worse, because it is silent. A caller holding a page when the shutdown lands will eventually give it back. That checkin arrives at a pool whose counters have been zeroed, so in_use goes to -1, and the page handle goes onto the free list. It points at a page in a browser that no longer exists. The next caller gets handed that, and finds out the hard way:

2) test a late checkin after a shutdown does not resurrect a dead page or corrupt state
   Assertion with == failed
   code:  assert Session.free_count("test") == 0
   left:  1
   right: 0

Stamp what you lend

I cannot stop a late checkin from arriving. The caller is a separate process, it holds the page, and it will hand it back whenever it is done, including long after I decided the browser was going away. What I can do is make the lateness visible.

Every checkout gets stamped with the generation of the browser it came from:

defp lend(state, page) do
  %{
    state
    | in_use: state.in_use + 1,
      checked_out: Map.put(state.checked_out, page.page_id, state.generation)
  }
end

Closing the browser bumps the generation, and the check on the way in is one line:

defp live_checkout?(state, page) do
  Map.get(state.checked_out, page.page_id) == state.generation
end

A checkin whose stamp does not match is dropped. Not an error, not a log to scroll past at three in the morning, just ignored, because that is exactly what it deserves. Shutdown also replies to every queued waiter with {:error, :shutdown} rather than leaving them to time out.

Generation counters are an old trick, and they turn up under other names, epochs and fencing tokens among them. The reason they keep coming back is that they turn an unanswerable question, is this handle still valid, into an integer comparison.

What the tests hold down

Three tests, and the names carry the whole specification:

test "a waiter that dies while queued does not shrink the pool" do
test "shutdown rejects queued waiters instead of abandoning them" do
test "a late checkin after a shutdown does not resurrect a dead page or corrupt state" do

All three fail against the old code, which is the only reason I trust them. The last one ends on the assertion I would keep if I could keep only one:

{:ok, fresh_page} = Session.checkout("test")
refute fresh_page.page_id == stale_page.page_id

Where I landed

Neither of these was a concurrency bug in the frightening sense. There was no race inside the GenServer, no shared mutable state, nothing the BEAM failed to protect me from. The mailbox did its job perfectly and delivered every message in order.

What the pool lacked was any way to tell whether what a message referred to still existed. A monitor covers one half of that by having the runtime tell me. A stamp covers the other by making the answer checkable on arrival. I now go looking for that gap whenever a GenServer hands something out and expects to get it back.