Fix infinite recursion and NameError on load when running with -rdebug

[native-api]

Description

Fixes #1107

When running with ruby -rdebug with a breakpoint set, self.to_str (which is not defined) is called repeatedly during module load, including the time after the class << self block is loaded but before further blocks that add more bits to self are.
As such, the dynamic method resolution machinery has to not break in this partially initialized state.

Todos

List any remaining work that needs to be done, i.e:

• Tests
  If this needs to be autotested, some pointers would be welcome: I've no idea how to load the module under test in a separate Ruby invocation from within RSpec.
• Documentation

[olleolleolle]

Style opinion: These guards would look easier to read as a "suffix if", like

return nil if name == :default_connection

[native-api]

Sure. I don't really have a preference here. I first tried name == :default_connection && return nil but that turned out to be invalid syntax ☹️

[iMacTia]

Thanks @native-api for trying to solve this, and apologies if I never came back to it since you initially raised the issue.

I'm still not entirely sure of what's is going on, but this solution of introducing guards looks more like a patch to me and it's not really elegant.
I don't think filling your code with guards due to some sort of loading race-condition is a good solution and I'd much rather consider others.

It might be worth investigating the issue a little more: you mentioned to_str is being called, but why is it going to method_missing? Can we do any change to our code to avoid it breaking? Maybe we're not following a convention or something? Can you share your findings so far so we can brainstorm together?

Last time I had a look (here), I did some progress by moving all the self.* methods into the class << self block. Although that didn't fix the problem entirely, it shows the kind of solution I would prefer. I feel like we're missing something 🤔

[native-api]

I'm still not entirely sure of what's is going on, but this solution of introducing guards looks more like a patch to me and it's not really elegant.
I don't think filling your code with guards due to some sort of loading race-condition is a good solution and I'd much rather consider others.

This happens because we initialize the nonexistent member resolution mechanism in parts rather than all at once: first override respond_to_missing? and missing_method in the class << self block; then add default_connection getter, and finally load faraday/connection with the Connection definition.
A module code is run block by block upon import like any other script: definitions are parsed and added as corresponding entitites, raw code is executed as it's encountered. So I wouldn't call it a "race condition", it's just the way how code is executed.
A debugger is likely calling <module>.to_str at every trace point -- probably to find out if it needs to break execution.

As such, an alternative solution is to initialize the entire mechanism all at once: define all the overriding methods inside the class << self block and require all the submodules before it.
But there's probably a reason why they are delay-loaded so I didn't want to touch that potential can of worms and offered a solution that requires minimal changes to code structure. It looks quite reasonable actually to first check if a member exists and only then use it.

[native-api]

Here's a trace of calls to the missing method resolution machinery during module load:

# I added `puts "<method_name>",<arg>.inspect` to the start of `respond_to_missing?` and `method_missing`
$ ruby -rdebug -e "require 'faraday'; puts 'bla-bla-bla'" 2>&1 <<!
b Rakefile:1
c
!
<...>
respond_to_missing?
:to_str

method_missing
:default_connection

respond_to_missing?
:to_str

respond_to_missing?
:to_str

bla-bla-bla

[iMacTia]

As such, an alternative solution is to initialize the entire mechanism all at once: define all the overriding methods inside the class << self block and require all the submodules before it.

If I got it correctly, moving the all the methods into the class << self block and leaving method_missing / respond_to_missing? at the very bottom of the block might solve the issue, right?
I'd very much like to test that solution (potentially on a separate PR, if you prefer to keep this one) and see if that works.

Should that not work, then looking at your last comment it seems like we might get away just with changing the implementation of respond_to_missing? to simply return false for to_str?

[native-api]

If I got it correctly, moving the all the methods into the class << self block and leaving method_missing / respond_to_missing? at the very bottom of the block might solve the issue, right?

No. You also need to require 'faraday/connection' (and anything else that might be needed for Connection.new() to work) before the class <<self block. I don't know if that will work without other changes (e.g. the submodules might expect something to be already initialized in the main module upon their load and malfunction somewhere down the line if that is not so) thus I don't want to pursue that way.

we might get away just with changing the implementation of respond_to_missing? to simply return false for to_str?

Yes. But I don't want to rely on the knowledge that it's specifically to_str being called -- since that may change in the future or in different circumstances (e.g. with a different debugger). I would very much prefer a universal solution that works regardless of what is called.

[native-api]

But I don't want to rely on the knowledge that it's specifically to_str being called

Not to mention that we might want to_str to be successfully resolved through default_connection as usual after the module's load.

[native-api]

I'd very much like to test that solution (potentially on a separate PR, if you prefer to keep this one) and see if that works.

Go ahead, knock yourself out 🙂 : #1204

As I suspected, some submodules expect the main module to already be initialized and break on load.

[iMacTia]

Sorry for being a pain 🙏 , that's far from what I really want, which is finding the right solution to the problem (if at all possible). I've left a comment in #1204 and opened #1205
If we're still unable to find an alternative solution, I'll reconsider this PR, so keep it warm for now 👍

[iMacTia]

Fixed in #1205 🎉
Thanks again @native-api for all the research 🙏