https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112022-05-28T04:16:13ZRuby Issue Tracking SystemRuby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=977822022-05-28T04:16:13Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>PR: <a href="https://github.com/ruby/ruby/pull/5967" class="external">https://github.com/ruby/ruby/pull/5967</a></p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=977892022-05-30T00:44:20Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>Since <code>Kernel#p</code> is a method for debugging, I think this spec would be useful. If it is made interruptable, it will be difficult to use <code>Kernel#p</code> in a block of <code>Thread.handle_interrupt(TimeoutError => :on_blocking)</code>.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=984092022-07-21T12:00:32Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>We discussed this issue at the dev meeting. We will add a document to <code>Kernel#p</code> so that it is uninterruptible and for debugging purpose.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=984342022-07-22T05:51:17Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/18">@mame (Yusuke Endoh)</a> I understand the discussion and I'm okay with the outcome, but I still don't understand why being uninterruptible matters in practice. I'm still a little concerned this can hang the interpreter, but I don't know for sure - because remember the internal write function can call the fiber scheduler. So it can potentially execute a lot of Ruby code in an uninterruptible way which is very unique - it is the only method which behaves like this.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=984372022-07-22T09:56:39ZEregon (Benoit Daloze)
<ul></ul><p>mame (Yusuke Endoh) wrote in <a href="#note-2">#note-2</a>:</p>
<blockquote>
<p>Since <code>Kernel#p</code> is a method for debugging, I think this spec would be useful. If it is made interruptable, it will be difficult to use <code>Kernel#p</code> in a block of <code>Thread.handle_interrupt(TimeoutError => :on_blocking)</code>.</p>
</blockquote>
<p>You mean if <code>p</code> is used inside a <code>Timeout.timeout</code> block?<br>
And you'd worry the object would be printed but no newline, or the write would be interrupted in the middle and have written some partial output (is that even possible when writing to a terminal given write is atomic in at least that case?)?<br>
If it's about the newline, that would be solved by <code>p</code> appending "\n" to the string and only doing a single write, everything is a million times simpler that way.</p>
<p>I think this is a very rare case and any developer must expect this or far worse effects of asynchronous exceptions when using <code>Timeout.timeout</code>/<code>Thread#raise</code>.</p>
<blockquote>
<p>We discussed this issue at the dev meeting. We will add a document to Kernel#p so that it is uninterruptible and for debugging purpose.</p>
</blockquote>
<p>IMHO this should not be part of specification, but an implementation choice/detail.<br>
So I'd suggest to write it like "Kernel#p is uninterruptible on CRuby to avoid being interrupted by Timeout.timeout" or so in the docs.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=984592022-07-26T08:34:49Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul></ul><p>My concern is that inserting <code>p(...)</code> changes a program behavior unintentionally (except that the p writes something to stdout, of course).</p>
<p>When debugging, we often insert <code>p(...)</code> and run the program to reproduce the bug being debugged. It would be annoying if the bug becomes unreproducible because the insertion of <code>p(...)</code> changes the behavior of the program. I don't want to see something like the "bug not reproduced on the debugger" problem.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=984752022-07-26T22:14:35Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><blockquote>
<p>My concern is that inserting p(...) changes a program behavior unintentionally (except that the p writes something to stdout, of course).</p>
</blockquote>
<p>There are so many ways it can do this. If the fiber scheduler is active it is a totally different code path. What about bufferred output (common on stdout).</p>
<p>What you really want is a blocking write to stderr, e.g. <code>fprintf(stderr, ...)</code>. This has very few side effects.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=985332022-07-30T11:38:41ZEregon (Benoit Daloze)
<ul></ul><p><a class="user active user-mention" href="https://bugs.ruby-lang.org/users/18">@mame (Yusuke Endoh)</a> Could you give an example in Ruby code?<br>
I don't understand your concern, of course <code>p</code> changes behavior because it calls <code>inspect</code> (which can do anything) and prints, but that seems fully expected.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=1036712023-06-22T23:27:01Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>I submitted a pull request to update the Kernel#p documentation: <a href="https://github.com/ruby/ruby/pull/7975" class="external">https://github.com/ruby/ruby/pull/7975</a></p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=1036722023-06-23T10:24:42ZEregon (Benoit Daloze)
<ul></ul><p><code>p</code> being uninterruptible means all of <code>object.inspect</code> and potential Fiber scheduler code is run under <code>Thread.handle_interrupt(Object => :never) { ... }</code>.<br>
That seems a really bad idea, because e.g. if <code>inspect</code> would take a long time (creates a huge String or buggy), then it's not possible to interrupt it, e.g. with Ctrl+C.<br>
I think this is a CRuby bug, it's too dangerous to disable interrupts for arbitrary code.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=1036732023-06-23T10:26:21ZEregon (Benoit Daloze)
<ul></ul><p>Mmh, but it does work to interrupt <code>p</code>:</p>
<pre><code>$ ruby -e 'o=Object.new; def o.inspect; loop { Thread.pass }; end; p 1; p o'
1
^C-e:1:in `pass': Interrupt
from -e:1:in `block in inspect'
from -e:1:in `loop'
from -e:1:in `inspect'
from -e:1:in `p'
from -e:1:in `<main>'
</code></pre>
<p>What am I missing then?</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=1036742023-06-23T10:34:08ZEregon (Benoit Daloze)
<ul></ul><p>It's because the code changed on master, <a href="https://github.com/ruby/ruby/commit/fe6b2e20e9f17ed2c2900aa72994e075ffdc7124" class="external">https://github.com/ruby/ruby/commit/fe6b2e20e9f17ed2c2900aa72994e075ffdc7124</a> had that bug but master doesn't call <code>inspect</code> under <code>rb_uninterruptible()</code>.<br>
On master:</p>
<pre><code class="c syntaxhl" data-language="c"><span class="k">static</span> <span class="n">VALUE</span>
<span class="nf">rb_f_p</span><span class="p">(</span><span class="kt">int</span> <span class="n">argc</span><span class="p">,</span> <span class="n">VALUE</span> <span class="o">*</span><span class="n">argv</span><span class="p">,</span> <span class="n">VALUE</span> <span class="n">self</span><span class="p">)</span>
<span class="p">{</span>
<span class="kt">int</span> <span class="n">i</span><span class="p">;</span>
<span class="k">for</span> <span class="p">(</span><span class="n">i</span><span class="o">=</span><span class="mi">0</span><span class="p">;</span> <span class="n">i</span><span class="o"><</span><span class="n">argc</span><span class="p">;</span> <span class="n">i</span><span class="o">++</span><span class="p">)</span> <span class="p">{</span>
<span class="n">VALUE</span> <span class="n">inspected</span> <span class="o">=</span> <span class="n">rb_obj_as_string</span><span class="p">(</span><span class="n">rb_inspect</span><span class="p">(</span><span class="n">argv</span><span class="p">[</span><span class="n">i</span><span class="p">]));</span>
<span class="n">rb_uninterruptible</span><span class="p">(</span><span class="n">rb_p_write</span><span class="p">,</span> <span class="n">inspected</span><span class="p">);</span>
<span class="p">}</span>
<span class="k">return</span> <span class="n">rb_p_result</span><span class="p">(</span><span class="n">argc</span><span class="p">,</span> <span class="n">argv</span><span class="p">);</span>
<span class="p">}</span>
<span class="k">static</span> <span class="n">VALUE</span>
<span class="nf">rb_p_result</span><span class="p">(</span><span class="kt">int</span> <span class="n">argc</span><span class="p">,</span> <span class="k">const</span> <span class="n">VALUE</span> <span class="o">*</span><span class="n">argv</span><span class="p">)</span>
<span class="p">{</span>
<span class="n">VALUE</span> <span class="n">ret</span> <span class="o">=</span> <span class="n">Qnil</span><span class="p">;</span>
<span class="k">if</span> <span class="p">(</span><span class="n">argc</span> <span class="o">==</span> <span class="mi">1</span><span class="p">)</span> <span class="p">{</span>
<span class="n">ret</span> <span class="o">=</span> <span class="n">argv</span><span class="p">[</span><span class="mi">0</span><span class="p">];</span>
<span class="p">}</span>
<span class="k">else</span> <span class="k">if</span> <span class="p">(</span><span class="n">argc</span> <span class="o">></span> <span class="mi">1</span><span class="p">)</span> <span class="p">{</span>
<span class="n">ret</span> <span class="o">=</span> <span class="n">rb_ary_new4</span><span class="p">(</span><span class="n">argc</span><span class="p">,</span> <span class="n">argv</span><span class="p">);</span>
<span class="p">}</span>
<span class="n">VALUE</span> <span class="n">r_stdout</span> <span class="o">=</span> <span class="n">rb_ractor_stdout</span><span class="p">();</span>
<span class="k">if</span> <span class="p">(</span><span class="n">RB_TYPE_P</span><span class="p">(</span><span class="n">r_stdout</span><span class="p">,</span> <span class="n">T_FILE</span><span class="p">))</span> <span class="p">{</span>
<span class="n">rb_uninterruptible</span><span class="p">(</span><span class="n">rb_io_flush</span><span class="p">,</span> <span class="n">r_stdout</span><span class="p">);</span>
<span class="p">}</span>
<span class="k">return</span> <span class="n">ret</span><span class="p">;</span>
<span class="p">}</span>
</code></pre>
<p>So only the write and the flush are uninterruptible now.<br>
So it does not seem correct to say <code>Kernel#p is uninterruptible</code>.</p>
<p>These rb_uninterruptible() seem to have very little effect, indeed it's only if there was a <code>Thread.handle_interrupt(TimeoutError => :on_blocking)</code> around the <code>p</code> and there is an interrupt and STDOUT is not just a TTY or a file where <code>write</code> cannot be interrupted anyway IIRC, then it could happen the interrupt is done during <code>p</code>'s <code>write</code> (or flush), instead of another blocking call in that block.<br>
I think that's fully expected, we are trying to tweak a corner of a very rare case by making things complicated, it seems not worth it.</p> Ruby master - Bug #18810: Make `Kernel#p` interruptable.https://bugs.ruby-lang.org/issues/18810?journal_id=1044142023-08-30T20:21:14Zjeremyevans (Jeremy Evans)code@jeremyevans.net
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li></ul><p>Applied in changeset <a class="changeset" title="Document that Kernel#p is for debugging and may be uninterruptible [ci skip] Fixes [Bug #18810]" href="https://bugs.ruby-lang.org/projects/ruby-master/repository/git/revisions/ae609a995e344877a990f4c16eca88b02dab5eba">git|ae609a995e344877a990f4c16eca88b02dab5eba</a>.</p>
<hr>
<p>Document that Kernel#p is for debugging and may be uninterruptible [ci skip]</p>
<p>Fixes [Bug <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Make `Kernel#p` interruptable. (Closed)" href="https://bugs.ruby-lang.org/issues/18810">#18810</a>]</p>