Project

General

Profile

Actions

Bug #18743

closed

Enumerator#next / peek re-use each others stacktraces

Added by sos4nt (Stefan Schüßler) almost 2 years ago. Updated 23 days ago.

Status:
Closed
Target version:
-
[ruby-core:108294]

Description

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.


Files

01-Recreate-stacktrace-enumerator.patch (1.29 KB) 01-Recreate-stacktrace-enumerator.patch marcper (Marcelo Pereira), 10/23/2022 01:09 PM

Updated by marcper (Marcelo Pereira) over 1 year ago

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

Updated by marcper (Marcelo Pereira) over 1 year ago

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

Updated by marcper (Marcelo Pereira) over 1 year ago

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

Updated by marcper (Marcelo Pereira) over 1 year ago

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.

Updated by marcper (Marcelo Pereira) about 1 year ago

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

Updated by marcper (Marcelo Pereira) about 1 year ago

@sos4nt (Stefan Schüßler) 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]

Updated by ko1 (Koichi Sasada) about 1 year ago

  • Assignee set to ko1 (Koichi Sasada)

Updated by matz (Yukihiro Matsumoto) about 1 year ago

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.

Updated by marcper (Marcelo Pereira) about 1 year ago

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

Updated by marcper (Marcelo Pereira) 12 months ago

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

Best,
Marcelo

Updated by marcper (Marcelo Pereira) 10 months ago

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

Updated by nobu (Nobuyoshi Nakada) 10 months ago

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).

Updated by marcper (Marcelo Pereira) 10 months ago

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.

Updated by nobu (Nobuyoshi Nakada) 10 months ago

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.

Updated by marcper (Marcelo Pereira) 9 months ago

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.

Updated by marcper (Marcelo Pereira) 8 months ago

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

Best

Updated by marcper (Marcelo Pereira) 5 months ago

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

Actions #19

Updated by nobu (Nobuyoshi Nakada) 5 months ago

  • 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

Updated by nagachika (Tomoyuki Chikanaga) 23 days ago

  • 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0