ruby / did_you_mean

The gem that has been saving people from typos since 2014
MIT License
1.87k stars 114 forks source link

Infinite loop #158

Open ldodds opened 3 years ago

ldodds commented 3 years ago

A Rails app I'm working on occasionally hangs, with the process becoming unresponsive and having to be killed.

We've also seen the same issue within the rails console and command-line scripts. In these cases a Ctrl-C causes the execution to either continue or end. But within the rails app we have to kill the process. We've removed some exception logging which was triggering the issue.

Using gdb and some manual testing, I've managed to identify that the issue is triggered whenever there is a NoMethodError generated by a call to an object and we are logging the exception message.

The gdb stack trace showed that a thread seems to be repeatedly calling methods in Correctable. When I remove the call to super.dup in to_s, to supply a hard-coded value instead, the issue went away.

We're currently on ruby-2.6.5 and Did You Mean 1.3.0.

I've tried upgrading to Did You Mean 1.5.0 but this doesn't seem to fix the issue. I also hit build/bundler issues similar to these which I've not been able to fully resolve.

We're looking to upgrade to newer ruby but I'm not sure if that will resolve the underlying problem as I gather its using version 1.5.0, but it might avoid the travis issues.

I've also been trying to disable the gem completely for the moment but setting RUBYOPT='--disable-did_you_mean' when running bundle exec rails s doesn't seem to work.

Any advice on other things I can try to either identify the cause or disable the gem?

yuki24 commented 3 years ago

Do you have a code snippet that replicates the issue? It's hard to investigate without code examples. Keep in mind that if you rescue Exception it will also rescue SignalException and Interrupt and the process wouldn't be able to handle signals as intended.

ldodds commented 3 years ago

I'll attempt to create a small test case, but may take me a little while to do that.

meinac commented 3 years ago

We are bitten by a similar problem exists in the grape-entity gem. I've already proposed a solution for it here: https://github.com/ruby-grape/grape-entity/pull/355.

The problem is, #to_s and #message methods(name_err_mesg_to_str) of the NameError call the rb_inspect(based on the object type) and if the #inspect raises NameError, then it introduces an infinite loop.

The following snippet demonstrates the issue;

class Foo
  def bar
    zoo_1
  rescue => e
    raise e.class, e.message
  end

  def zoo
    true
  end

  def inspect
    bar
  end
end

Foo.new.bar

I think adding a warning to the documentation of the Kernel#inspect method would be nice.

ldodds commented 3 years ago

Based on @meinac comments and some further testing I'm not sure that the issue is with did you mean and instead related to timing and errors with #to_s/#inspect.

@yuki24 if I comment out this line in the did you mean gem, then I assume it wont ever be activated for a NoMethodError?

  correct_error NoMethodError, MethodNameChecker

If so, then our problem is elsewhere and I can close this bug.

ldodds commented 3 years ago

Our issue ended up not being with did you mean. The comments from @meinac were enough to track down our error which was actually due to issues with #to_s/#inspect calls.

We have a large data structure that was being held as a member variable of an object. Attempting to dump that via to_s or inspect is significantly slow. We don't do that normally.

Logging a NoMethodError from an object (in this case a Rails controller) that held an indirect reference to one of these structures caused the exception handling to dump those objects as part of calling #inspect somewhere. We've implemented #inspect on the relevant object to avoid logging the internals of the data structure and this fixes the problem. No further blocks which were presenting themselves as stuck processes.

I'm not sure whether there is a still an infinite loop issue as @meinac reports, so will leave this up to maintainer to close. But my original issue is resolved. Sorry for the noise!

meinac commented 3 years ago

Thanks for double checking @ldodds. As you've mentioned, this is a problem with Ruby and not did_you_mean.

I am planning to propose a solution for Ruby like this or at least a warning message for the Kernel#inspect documentation. WDYT @yuki24?

vakuum commented 1 year ago

@ldodds: Does this commit https://github.com/rails/rails/commit/080edb444a16e759a84e0f46db8970faed00fca7 in Rails 6.1 solve the problem?