Skip to content

don't swallow stack traces - #64

Open
jeremylightsmith wants to merge 1 commit into
safwank:masterfrom
mechanical-orchard:dont-swallow-stacktraces
Open

don't swallow stack traces#64
jeremylightsmith wants to merge 1 commit into
safwank:masterfrom
mechanical-orchard:dont-swallow-stacktraces

Conversation

@jeremylightsmith

Copy link
Copy Markdown

this is a breaking change, but it will allow exceptions to retain their original stack traces when a retry fails

@snackycracky

Copy link
Copy Markdown

There's an inconsistency: only the exception path is changed to {e, stacktrace}, while the retriable-value path ({:retriable, e}) still passes the bare value. So an else clause now has to handle two different input shapes depending on whether the failure was a thrown exception or a returned value. The default clause only handles that by luck of the is_exception/1 guard falling through.

We can keep the stacktrace internal and reraise it only where the bug actually lives — the default else. User-supplied else clauses keep receiving the bare e, exactly as before.

exception_clause =
  if Keyword.has_key?(clauses, :else) do
    # user-supplied else: keep the historical contract (bare exception)
    quote do
      {:exception, e, _stacktrace} ->
        case e do
          unquote(else_clause)
        end
    end
  else
    # default: reraise with the original stacktrace instead of `raise e`
    quote do
      {:exception, e, stacktrace} ->
        reraise e, stacktrace
    end
  end

plus the one-line internal change in block_runner/2:

-  {:cont, {:exception, e}}
+  {:cont, {:exception, e, __STACKTRACE__}}

The {:exception, e, stacktrace} 3-tuple is purely internal and never reaches user code.

Why it's better

this PR non-breaking version
Original stacktrace preserved
Breaking change / major bump ❌ required ✅ avoided
Existing else tests need editing ❌ yes ✅ untouched
Heterogeneous else input ❌ yes ✅ no

The stacktrace test still passes unchanged:

test "reraises with the original stacktrace when giving up on an exception" do
  formatted =
    try do
      retry with: [1] do
        boom()
      end
    rescue
      e -> Exception.format(:error, e, __STACKTRACE__)
    end

  assert formatted =~ "(RuntimeError) boom"
  assert formatted =~ "RetryTest.boom"   # points at the failing site, not retry.ex
end

The one thing this PR gives that the non-breaking version doesn't: exposing the stacktrace to custom else clauses. If that's a goal, I'd suggest doing it additively (an opt-in second-element form) in a follow-up, rather than changing the shape everyone already depends on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants