-
Notifications
You must be signed in to change notification settings - Fork 631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
engine: make output Readline-friendly #536
Conversation
The motivation behind this commit is to make Foreman respect GNU Readline [1]. As Foreman is often used with Pry (or at least many people want to), and Pry is dependent on Readline, using these tools together is very problematic [2][3]. The problem is that when you start Pry by means of Foreman, Foreman supresses output from Pry, so you don't see what you type. The output can only be seen after pressing carriage return. Demonstration (a bit hard to follow, but you may want to try it yourself; clone the Pry repo and execute the same command): http://showterm.io/3eb716461d6602ac90b09 Although this problem was reported by Pry users, any Readline application is affected. This commit uses IRB for tests, because it also depends on Readline, hence it suits for testing perfectly. Other Readline dependent applications that I tested were GHCi, the Lua REPL, Eshell (erlang shell). Previously, Pry was using a trick, which allowed us to bypass this problem [4]. The fix was featured in Pry v0.9.12.6, but then it was removed by accident. It's been readded on HEAD recently (not released yet). However, the fix we currently have is still not perfect. Although you can see your input immediately, you can't see your output now :D Demo: http://showterm.io/f648de27568d96a02f1b3 The fix breaks correct piping. Now, when you pipe Pry's output, it doesn't output Readline's prompt and user's input. What's being piped is only return values of executed expressions. Demo: http://showterm.io/3651389faf1fdc0d18211 To fix the missing output and pipe everything, including the user's input, we need to set `Readline.output` correctly. However, if we do that, we break minimal support for Foreman. So when I fixed Pry [5], I figured it would be interesting to try to fix Foreman. Now, it's time to talk about the changes here. I'm not very familiar with Foreman, so take the code with a grain of salt. Basically, the whole point of the fix is to read one character at a time and print it immediately, instead of waiting for a newline character. This technique allows us to prepend so-called markers (timestamps, if you wish) to a Readline prompt and display the user's input. It works for all the Readline apps that I mentioned above and doesn't break piping (that is, tee). Note. I had to remove `$stdout.flush` from the output method, because it was causing thread deadlocks with the flush in `#handle_io`. The deadlock appears when you use Foreman with Pry, pipe the output and trigger SIGINT (control-c press). Example: % foreman start -f Procfile | tee log 15:50:05 pryhere.1 | started with pid 26881 15:50:28 pryhere.1 | [1] pry(main)> <CTRL+C><CTRL+D> 15:50:49 pryhere.1 | [2] pry(main)> foreman/engine.rb:323:in `synchronize': deadlock; recursive locking (ThreadError) This is probably because of the race condition, when on SIGINT Foreman prints stuff here [6], which flushes $stdout. [1]: http://ruby-doc.org/stdlib-2.2.0/libdoc/readline/rdoc/Readline.html [2]: pry/pry#1290 [3]: pry/pry#1275 (comment) [4]: https://github.com/pry/pry/blob/f0cbec507111743fdbc273735ea4f0f6164a5b21/lib/pry/repl.rb#L184 [5]: pry/pry#1372 [6]: https://github.com/ddollar/foreman/blob/339ff1df2347328134e471e413ad70766058faa3/lib/foreman/engine.rb#L413
Foreman intentionally does not handle stdin on the terminal. If you are running multiple processes which of them should get the data from stdin? |
That's a very good point. I just tested both IRB and Pry together in one Procfile and it is unusable in both cases, with and without this patch (in a slightly different manner, though). I cannot easily describe the behaviour, because it's unpredictable. I assume if you add multiple applications that read stdin to Procfile, you know what you're doing. On the other hand, maybe it makes sense to read input sequentially from each application. So the first time I enter input for Pry, press Enter, then I enter input for IRB, then for Pry again. Among Rubyists Pry is probably the only thing that reads from stdin. If you think this is still a bad idea, would you be so kind and drop a comment here explaining the situation, so I could close this issue and label it "WONTFIX" :) P.S.: the Travis fail shouldn't be hard to fix, they just set their custom prompt. |
I haven't read through this in detail nor researched this issue a lot, but I have a couple of comments.
|
I'd love to have support for pry when using foreman. Debugging is of course very useful when developing applications but foreman doesn't handle it well. Maybe this isn't the way to fix it, but I'd love to see something done about it. |
I don't think it makes sense to send stdin to all processes since that will almost certainly never be what you want. Your typing is intended for one process. My advice would be to not run |
In order to support it we needed to patch Foreman. However, the Foreman author rejected it. Details: ddollar/foreman#536
@ddollar Note that most people is not adding |
+1 |
2 similar comments
+1 |
+1 |
👍 @ddollar, any chance of reconsidering based on @deivid-rodriguez's message? This is indeed an extremely common use case for pry and it's a shame to have to keep the main server process out of foreman in order to attach a debugger. |
+1 I've been using (edit: I mistyped the pry command) |
Guys, have you actually tried this patch and verify it works? If not, could someone do it? |
Yes, I have tested this patch and it does work for me. |
Cool. |
+1 @deivid-rodriguez thanks for spelling out the main use case for this so that @ddollar can review this. Lack of binding.pry support is one of the primary reasons I use standard |
For those looking for a workaround to using pry with foreman: pry-remote. The biggest caveat for me is that tab completion doesn't work. |
👍 |
I couldn't get pry-remote working, but I got this patch working by using kyrylo's branch of foreman. This is how:
It's so great to be able to see what I'm typing again! |
I strongly believe that all Ruby REPLs should read/write from |
It'd be great if we could reconsider this. The screwy interaction with pry is the only downside of foreman. |
@drewish The problem is with pry, not foreman. Foreman has no way to demultiplex the standard I/O with multiple children. The solution is for only one process at a time to run pry against the terminal using IO.console (see above) or to directly run the command from the Procfile without forman while using pry. You can see a similar problem running better_errors against multiple web processes. |
pkgsrc change: add support for pkg_alternatives ### HEAD #### Features * Add Pry::Testable, an improved modular replacement for PryTestHelpers. **breaking change**. See pull request [#1679](pry/pry#1679). * Add a new category module: "Pry::Platform". Loosely related to #1668 below. See pull request [#1670](pry/pry#1670) * Add `mac_osx?` and `linux?` utility functions to Pry::Helpers::BaseHelpers. See pull request [#1668](pry/pry#1668). * Add utility functions for drawing colorised text on a colorised background. See pull request [#1673](pry/pry#1673). #### Bug fixes * Fix a case of infinite recursion in `Pry::Method::WeirdMethodLocator#find_method_in_superclass` that users of the [Hanami](http://hanamirb.org/) web framework experienced and reported since 2015. See pull request [#1639](pry/pry#1689). * Fix a bug where Method objects were not returned for setters inherited from a default (Pry::Config::Default). Eg, this is no longer an error: pry(main)> d = Pry::Config.from_hash({}, Pry::Config::Default.new) pry(main)> d.method(:exception_whitelist=) # Error See pull request [#1688](pry/pry#1688). * Do not capture unused Proc objects in Text helper methods `no_color` and `no_paging`, for performance reasons. Improve the documentation of both methods. See pull request [#1691](pry/pry#1691). * Fix `String#pp` output color. See pull request [#1674](pry/pry#1674). ### 0.11.0 * Add alias 'whereami[?!]+' for 'whereami' command. ([#1597](pry/pry#1597)) * Improve Ruby 2.4 support ([#1611](pry/pry#1611)): * Deprecated constants are hidden from `ls` output by default, use the `-d` switch to see them. * Fix warnings that originate in Pry while using the repl. * Improve completion speed in large applications. ([#1588](pry/pry#1588)) * Pry::ColorPrinter.pp: add `newline` argument and pass it on to PP. ([#1603](pry/pry#1603)) * Use `less` or system pager pager on MS Windows if it is available. ([#1512](pry/pry#1512)) * Add `Pry.configure` as an alternative to the current way of changing configuration options in `.pryrc` files. ([#1502](pry/pry#1502)) * Add `Pry::Config::Behavior#eager_load!` to add a possible workaround for issues like ([#1501](pry/pry#1501)) * Remove Slop as a runtime dependency by vendoring v3.4 as Pry::Slop. People can depend on Slop v4 and Pry at the same time without running into version conflicts. ([#1497](pry/pry#1497)) * Fix auto-indentation of code that uses a single-line rescue ([#1450](pry/pry#1450)) * Remove "Pry::Config#refresh", please use "Pry::Config#clear" instead. * Defining a method called "ls" no longer breaks the "ls" command ([#1407](pry/pry#1407)) * Don't raise when directory permissions don't allow file expansion ([#1432](pry/pry#1432)) * Syntax highlight <tt> tags in documentation output. * Add support for BasicObject subclasses who implement their own #inspect (#1341) * Fix 'include RSpec::Matchers' at the top-level (#1277) * Add 'gem-readme' command, prints the README file bundled with a rubygem * Add 'gem-search' command, searches for a gem with the rubygems.org HTTP API * Fixed bug in the `cat` command where it was impossible to use line numbers with files ([#1349](pry/pry#1349)) * Fixed uncaught Errno::EOPNOTSUPP exception when $stdout is a socket ([#1352](pry/pry#1352)) * Display a warning when you cd'ed inside a C object and executed 'show-source' without arguments ([#691](pry/pry#691)) * Make the stagger_output method more reliable by reusing possibly available Pry instance ([#1364](pry/pry#1364)) * Make the 'gem-install' message less confusing by removing backticks ([#1350](pry/pry#1350)) * Fixed error when Pry was trying to load incompatible versions of plugins ([#1312](pry/pry#1312)) * Fixed bug when `hist --clear` led to ArgumentError ([#1340](pry/pry#1340)) * Fixed the "uninitialized constant Pry::ObjectPath::StringScanner" exception during autocomplete ([#1330](pry/pry#1330)) * Secured usage of colours with special characters (RL_PROMPT_START_IGNORE and RL_PROMPT_END_IGNORE) in Pry::Helpers::Text ([#493](pry/pry#493 (comment))) * Fixed regression with `pry -e` when it messes the terminal ([#1387](pry/pry#1387)) * Fixed regression with space prefixes of expressions ([#1369](pry/pry#1369)) * Introduced the new way to define hooks for commands (with `Pry.hooks.add_hook("{before,after}_commandName")`). The old way is deprecated, but still supported (with `Pry.commands.{before,after}_command`) ([#651](pry/pry#651)) * Removed old API's using `Pry::Hooks.from_hash` altogether * Removed hints on Foreman support (see [this](ddollar/foreman#536)) * Fixed support for the tee command ([#1334](pry/pry#1334)) * Implemented support for CDPATH for ShellCommand ([#1433](pry/pry#1433), [#1434](pry/pry#1434)) * `Pry::CLI.parse_options` does not start Pry anymore ([#1393](pry/pry#1393)) * The gem uses CPU-less platforms for Windows now ([#1410](pry/pry#1410)) * Add `Pry::Config::Memoization` to make it easier to implement your own `Pry::Config::Default` class.([#1503](pry/pry#1503)) * Lazy load the config defaults for `Pry.config.history` and `Pry.config.gist`.
The motivation behind this commit is to make Foreman respect
GNU Readline 1.
As Foreman is often used with Pry (or at least many people want to), and
Pry is dependent on Readline, using these tools together is very
problematic 2,3.
The problem is that when you start Pry by means of Foreman, Foreman
supresses output from Pry, so you don't see what you type. The output
can only be seen after pressing carriage return. Demonstration (a bit
hard to follow, but you may want to try it yourself; clone the Pry repo
and execute the same command):
http://showterm.io/3eb716461d6602ac90b09
Although this problem was reported by Pry users, any Readline
application is affected. This commit uses IRB for tests, because it also
depends on Readline, hence it suits for testing perfectly. Other
Readline dependent applications that I tested were GHCi, the Lua REPL,
Eshell (erlang shell).
Previously, Pry was using a trick, which allowed us to bypass this
problem 4. The fix was featured in Pry v0.9.12.6, but then it was
removed by accident. It's been readded on HEAD recently (not released
yet). However, the fix we currently have is still not perfect. Although
you can see your input immediately, you can't see your output now :D
Demo:
http://showterm.io/f648de27568d96a02f1b3
The fix breaks correct piping. Now, when you pipe Pry's output, it
doesn't output Readline's prompt and user's input. What's being piped is
only return values of executed expressions.
Demo:
http://showterm.io/3651389faf1fdc0d18211
To fix the missing output and pipe everything, including the user's
input, we need to set
Readline.output
correctly. However, if we dothat, we break minimal support for Foreman. So when I fixed Pry 5, I
figured it would be interesting to try to fix Foreman.
Now, it's time to talk about the changes here. I'm not very familiar
with Foreman, so take the code with a grain of salt. Basically, the
whole point of the fix is to read one character at a time and print it
immediately, instead of waiting for a newline character. This technique
allows us to prepend so-called markers (timestamps, if you wish) to a
Readline prompt and display the user's input. It works for all the
Readline apps that I mentioned above and doesn't break piping (that is,
tee).
Note. I had to remove
$stdout.flush
from the output method, because itwas causing thread deadlocks with the flush in
#handle_io
. Thedeadlock appears when you use Foreman with Pry, pipe the output and
trigger SIGINT (control-c press). Example:
% foreman start -f Procfile | tee log
15:50:05 pryhere.1 | started with pid 26881
15:50:28 pryhere.1 | 1 pry(main)> <CTRL+C><CTRL+D>
15:50:49 pryhere.1 | 2 pry(main)> foreman/engine.rb:323:in
`synchronize': deadlock; recursive locking (ThreadError)
This is probably because of the race condition, when on SIGINT Foreman
prints stuff here 6, which flushes $stdout.