Address review: raise RuntimeError on subsequent empty cycle, simplify test

Author: lukaszsamson

lib/elixir/lib/stream.ex

@@ -1437,7 +1437,7 @@ defmodule Stream do
         do_cycle(cycle, [], cycle, fun.(element, acc), fun)
 
       {_, []} ->
-        do_cycle(check_cycle_first_element(cycle), [], cycle, {:cont, acc}, fun)
+        do_cycle(check_cycle_subsequent_element(cycle), [], cycle, {:cont, acc}, fun)
     end
   end
 
@@ -1446,10 +1446,26 @@ defmodule Stream do
   end
 
   defp check_cycle_first_element(reduce) do
+    check_cycle_non_empty(
+      reduce,
+      ArgumentError,
+      "cannot cycle over an empty enumerable"
+    )
+  end
+
+  defp check_cycle_subsequent_element(reduce) do
+    check_cycle_non_empty(
+      reduce,
+      RuntimeError,
+      "cycled enumerable became empty after a previous iteration produced elements"
+    )
+  end
+
+  defp check_cycle_non_empty(reduce, exception, message) do
     fn acc ->
       case reduce.(acc) do
         {state, []} when state in [:done, :halted] and elem(acc, 0) != :halt ->
-          raise ArgumentError, "cannot cycle over an empty enumerable"
+          raise exception, message
 
         other ->
           other

lib/elixir/test/elixir/stream_test.exs

@@ -281,41 +281,26 @@ defmodule StreamTest do
              [1, 1, 1, 1, 1]
   end
 
-  test "cycle/1 raises and does not infinite-loop when a subsequent reduce yields no elements" do
-    {:ok, agent} = Agent.start_link(fn -> 0 end)
+  test "cycle/1 raises when a subsequent reduce yields no elements" do
+    Process.put(:cycle_counter, 0)
 
     stream =
       Stream.resource(
-        fn -> Agent.get_and_update(agent, fn n -> {n, n + 1} end) end,
+        fn ->
+          n = Process.get(:cycle_counter)
+          Process.put(:cycle_counter, n + 1)
+          n
+        end,
         fn
           0 -> {[:a], :done}
           _ -> {:halt, :ok}
         end,
         fn _ -> :ok end
       )
 
-    parent = self()
-
-    task =
-      Task.async(fn ->
-        try do
-          Stream.cycle(stream) |> Enum.take(3)
-        rescue
-          e in ArgumentError -> send(parent, {:raised, e.message})
-        end
-      end)
-
-    try do
-      case Task.yield(task, 1000) || Task.shutdown(task, :brutal_kill) do
-        {:ok, _} ->
-          assert_received {:raised, "cannot cycle over an empty enumerable"}
-
-        nil ->
-          flunk("Stream.cycle/1 entered an unbounded loop with no progress")
-      end
-    after
-      Agent.stop(agent)
-    end
+    assert_raise RuntimeError,
+                 "cycled enumerable became empty after a previous iteration produced elements",
+                 fn -> Stream.cycle(stream) |> Enum.take(3) end
   end
 
   test "dedup/1 is lazy" do