igrigorik/em-synchrony — Issue #84: A few concerns about the method replacement pattern
Author: slyphon
Created: 2011-11-13 19:06:00+00:00
State: open

I've been working on integrating EM::Synchrony/Fiber-style interfaces into several EM-based libraries I'm developing/maintaining, and I wanted to bring up for discussion some concerns I have about promoting the ["replace methods with synchrony versions"](https://github.com/igrigorik/em-synchrony/blob/master/lib/em-synchrony/em-http.rb) pattern.

The main concern I have is that this is clearly [Action at a distance][]. I've seen this in my own code, and it introduces bugs that can be difficult to track down. The main problem with the method-replacement style is that it works _if none of the methods being replaced depend on another method being replaced_.

Let's say I have the following (contrived) class:

```ruby
class Foo < Blah
def bark
EM::DefaultDeferrable.new.tap do |dfr|
EM.next_tick do
dfr.succeed("woof!")
end
end
end

def bark_loudly
EM::DefaultDeferrable.new.tap do |dfr|
bark.callback do |str|
dfr.callback(str.upcase)
end
end
end
end
```

This is a fairly common pattern, the problem comes when someone replaces the `bark` and `bark_loudly` methods naievly.

```ruby
class Foo < Blah
%w[bark bark_loudly].each do |name|
class_eval(<<-EOS, __FILE__, __LINE__+1)
alias :"a#{name}" :#{name}
def #{name}
EM::Synchrony.sync(a#{name})
end
EOS
end
end
```

This will work in the case of the `bark` method, but `bark_loudly` will break because it's calling `bark` expecting to get a Deferrable back, but instead it gets a string.

(As a side note, the `alias` line above will cause problems if the class is reloaded, as it will overwrite the original `abark` method with the now-sync `bark` method. A guard should be added to ensure that `abark` isn't already defined.)

I've had this happen to me [when using the WebMock library][], as it (somewhat frustratingly) rewrites `HTTPMethods` if `Synchrony` is defined. My code is depending on the asynchronous nature of `EM::HttpRequest`, and their assumption breaks my code. Not only that, it breaks my code in a way that's very difficult for me to track down, because no exception is thrown, it's just my code hangs forever. Now, I fully understand that this isn't part of em-synchrony, however I'm concerned because I think that em-synchrony is an important project, and I feel promoting this kind of pattern may cause more problems than it solves.

Unless all of the libraries being used understand how to communicate with the EM::Synchrony-rewritten methods, then there will be problems. What I've taken to doing in my own code is writing a [synchrony-aware wrapper-class][] around the async class. Both the [sync][] and [async][] classes define `#to_synchrony` and `#to_async` methods which allow easy switching between styles. This preserves the original methods, and will not create "spooky action at a distance" in code that doesn't know about Synchrony, and yet will make it convenient for people who want to have easy access to the sync version.

The wrapper style isn't as slick as method replacement because it's somewhat more explicit, however, implicitly changing the definition of methods is changing global state in a way that can have unforseen consequences.

[Action at a distance]: http://en.wikipedia.org/wiki/Action_at_a_distance_(computer_programming)
[when using the Webmock library]: https://github.com/bblimke/webmock/blob/master/lib/webmock/http_lib_adapters/em_http_request/em_http_request_1_x.rb#L30
[synchrony-aware wrapper-class]: https://github.com/slyphon/zk-eventmachine/blob/dev/synchrony/lib/z_k/z_k_event_machine/synchrony_client.rb#L25
[async]: https://github.com/slyphon/zk-eventmachine/blob/dev/synchrony/lib/z_k/z_k_event_machine/client.rb#L148
[sync]: https://github.com/slyphon/zk-eventmachine/blob/dev/synchrony/lib/z_k/z_k_event_machine/synchrony_client.rb#L79