Bug #18743: Enumerator#next / peek re-use each others stacktraces

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: -
• Backport: 3.0: REQUIRED, 3.1: REQUIRED, 3.2: DONE

[sos4nt (Stefan Schüßler)]

I encountered an odd behavior.

If I rescue the StopIteration exception from peek and call next afterwards: (or vice-versa)

# enum.rb             # 1
                      # 2
enum = [].each        # 3
enum.peek rescue nil  # 4
enum.next             # 5

it will show the stacktrace from the rescued peek call:

$ ruby enum.rb
enum.rb:4:in `peek': iteration reached an end (StopIteration)
	from enum.rb:4:in `<main>'

Whereas the error should refer to next on line number 5.

The same happens when calling peek after next or when having muliple peek / next calls:

# enum.rb                # 1
                         # 2
enum = [].each           # 3
enum.peek rescue nil     # 4
enum.next rescue nil     # 5
enum.peek rescue nil     # 6
puts "line #{__LINE__}"  # 7
enum.next                # 8

The stacktrace from the first (rescued) peek or next call will be shown which doesn't reflect the actual error location:

$ ruby enum.rb
line 7
enum.rb:4:in `peek': iteration reached an end (StopIteration)
	from enum.rb:4:in `<main>'

This is very confusing when debugging code.

[marcper (Marcelo Pereira)]

This issue is part of Ruby for a long time now:
https://github.com/ruby/ruby/commit/3a855da47b867e24428582b0a20aa1ea7e4dc2af

Proposed fix here:
https://github.com/ruby/ruby/pull/6201

[marcper (Marcelo Pereira)]

Good day, @matz (Yukihiro Matsumoto). Is the fix above acceptable?

[marcper (Marcelo Pereira)]

Hello @matz (Yukihiro Matsumoto), please let me know if someone else should be pinged for this.

[marcper (Marcelo Pereira)]

• File 01-Recreate-stacktrace-enumerator.patch 01-Recreate-stacktrace-enumerator.patch added

@matz (Yukihiro Matsumoto), I'm adding the patch file here as well, following the contributor's guide.

[marcper (Marcelo Pereira)]

Pinging @matz (Yukihiro Matsumoto). Sorry if I'm being annoying, but the contributor guide says I should send a reminder after a few weeks if there's no response.

[marcper (Marcelo Pereira)]

Good evening, @matz (Yukihiro Matsumoto). The PR with the bug fix is available now for a few months. Please advise on the next steps.

[marcper (Marcelo Pereira)]

@sos4nt could you please assign this issue to @matz (Yukihiro Matsumoto)? This is a core class problem, and he is the listed maintainer for it.

I can confirm the bug still exists in ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-linux]

[ko1 (Koichi Sasada)]

• Assignee set to ko1 (Koichi Sasada)

[matz (Yukihiro Matsumoto)]

It should be fixed. But I think the proposed change https://github.com/ruby/ruby/pull/6201 is not enough, we should link the old exception with the new one (via cause).

Matz.

[marcper (Marcelo Pereira)]

Hello again. I've modified the patch to also link to the old exception via cause. Let me know if it's now a reasonable solution.

Thanks,
Marcelo

[marcper (Marcelo Pereira)]

Hello @ko1 (Koichi Sasada), let me know if the patch in the current form is acceptable.

Best,
Marcelo

[marcper (Marcelo Pereira)]

Hey again, @ko1 (Koichi Sasada). This is just a reminder that the patch is pushed to Github and is waiting for a decision.

Best,
Marcelo

[nobu (Nobuyoshi Nakada)]

Seems find to me.
One thing I concerned, stop_exc will be re-created every times, and no way to know where it was first used up.
How about chaining by cause?

diff --git a/enumerator.c b/enumerator.c
index b33c1713718..1cc8f9108cf 100644
--- a/enumerator.c
+++ b/enumerator.c
@@ -30,6 +30,9 @@
 #include "internal/rational.h"
 #include "ruby/ruby.h"
 
+extern ID ruby_static_id_cause;
+#define id_cause ruby_static_id_cause
+
 /*
  * Document-class: Enumerator
  *
@@ -787,8 +790,16 @@ get_next_values(VALUE obj, struct enumerator *e)
 {
     VALUE curr, vs;
 
-    if (e->stop_exc)
-        rb_exc_raise(e->stop_exc);
+    if (e->stop_exc) {
+        VALUE exc = e->stop_exc;
+        VALUE result = rb_attr_get(exc, id_result);
+        VALUE mesg = rb_attr_get(exc, idMesg);
+        if (!NIL_P(mesg)) mesg = rb_str_dup(mesg);
+        VALUE stop_exc = rb_exc_new_str(rb_eStopIteration, mesg);
+        rb_ivar_set(stop_exc, id_result, result);
+        rb_ivar_set(stop_exc, id_cause, exc);
+        rb_exc_raise(stop_exc);
+    }
 
     curr = rb_fiber_current();
 

In the test, save $! at the first rescue then assert_same(cause, exc.cause).

[marcper (Marcelo Pereira)]

The exceptions are already being chained through cause, but I've just updated the tests to make this fact clearer.

Indeed a new exception is created every time. Initially I looked for an API to simply update the backtrace on the existing exception, and couldn't find one. But given the need to chain exceptions, creating a new one every time turned out to be the right approach.

Thank you for reviewing this, @nobu (Nobuyoshi Nakada), and I'm happy to address any other concerns.

[nobu (Nobuyoshi Nakada)]

Your PR makes stop_exc to be overwritten for each StopIteration.
IMO, it should not change and the cause of all subsequent StopIterations should be the same first exception.

[marcper (Marcelo Pereira)]

Oh, I understood from Matz's initial suggestion that the exceptions should be chained, but I see now what he meant. I've applied the changes you suggested, @nobu (Nobuyoshi Nakada). Thank you for clarifying, and for reviewing.

[marcper (Marcelo Pereira)]

Hello again, @ko1 (Koichi Sasada). Please let me know if this patch is acceptable. I followed the suggestions from @nobu (Nobuyoshi Nakada) already.

Best

[marcper (Marcelo Pereira)]

Hi @nobu (Nobuyoshi Nakada), and @ko1 (Koichi Sasada). The change was merged in July. Shouldn't this issue be closed?

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed
• Backport changed from 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN to 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED to 3.0: REQUIRED, 3.1: REQUIRED, 3.2: DONE

ruby_3_2 5e4606423da96c2e26b00981c22bddfce0d970bf merged revision(s) f15123c34ce80f3928612befe2a9aaf4c9d27fda.