Future and nil values

[mrbrdo]

There is a "problem" when a Future refers to a nil result (or false). The Future will seem like it is a NilClass object since Future is a BasicObject and will proxy almost everything to NilClass. However Future is truthy and not falsey like NilClass. This can be a huge pain to debug and figure out.

Maybe I can recommend some possible solutions. Please note I don't know this library very well.

1. Define #class and #inspect and possibly a few other methods on Future, or at least inspect if you/people rely on class being the underlying class. Personally I would avoid doing it this way with BasicObject though as it is too much magic and very confusing.
2. If I understand correctly, the future will be evaluated here: https://github.com/godfat/rest-core/blob/master/lib/rest-core/client.rb#L170 In this case it might be a good idea to return the actual object from this method instead of the future, so as to not leak out these "weird" objects. If the future has been resolved anyway there is no point in keeping the Future object anyway, you just want the result. Of course since instance_variable_get is also proxied you will need some kind of accessor method for that.

If possible it would be better to use SimpleDelegator from standard library to avoid this problem.

[godfat]

There is a "problem" when a Future refers to a nil result (or false). The Future will seem like it is a NilClass object since Future is a BasicObject and will proxy almost everything to NilClass. However Future is truthy and not falsey like NilClass. This can be a huge pain to debug and figure out.

[...]

If possible it would be better to use SimpleDelegator from standard library to avoid this problem.

I agree this could be a potential problem, but using SimpleDelegator does not help this:

if SimpleDelegator.new(nil); true; else false; end # => true

The only way to fix this might be: Never use this kind of proxy :) But that could change a lot of things...

Define #class and #inspect and possibly a few other methods on Future, or at least inspect if you/people rely on class being the underlying class. Personally I would avoid doing it this way with BasicObject though as it is too much magic and very confusing.

Honestly I did try to use something other than BasicObject, but in the end I got a ton of corner cases and eventually realized that BasicObject works better. I don't remember the detail, but it would be great if you could provide some more clues about not using BasicObject, so that we could revisit this easier?

We have the following options:

• BasicObject as for now
• SimpleDelegator, which is a more completed implementation than the current BasicObject
• BlankSlate as in some other libraries (i.e. undefine some methods in Object)
• Something less transparent for not undefining methods
• Make Future explicit (i.e. we always call load or get or read or call to get the response)

If I understand correctly, the future will be evaluated here: https://github.com/godfat/rest-core/blob/master/lib/rest-core/client.rb#L170 In this case it might be a good idea to return the actual object from this method instead of the future, so as to not leak out these "weird" objects. If the future has been resolved anyway there is no point in keeping the Future object anyway, you just want the result. Of course since instance_variable_get is also proxied you will need some kind of accessor method for that.

It's not evaluated at that time. It's only evaluated when someone tries to read the response body. (or header) You could check this for more detail: https://github.com/godfat/rest-core/blob/rest-core-3.3.0/lib/rest-core/promise.rb#L42-L54

Please let me know if you couldn't understand the code. I would try to explain the architecture here.

[mrbrdo]

@godfat I did not mean to use SimpleDelegator to make it behave as falsey. However SimpleDelegator will at least properly return #class etc. It is not possible in Ruby to make an object behave falsey. The problem with BasicObject that you are using is that it is incredibly hard to figure out what's going on from a developer's perspective. I can't find any good example but in my experience it's usually best to avoid it especially since SimpleDelegator is provided by stdlib for this purpose and is less confusing. I am not familiar with BlankSlate but it could also be an option. The problem is not so much that the result doesn't behave falsey but more that it looks like you have a NilClass when in fact you don't.

The reason I assumed the Future is evaluated because you call kind_of? on it in that code. That will be proxied through https://github.com/godfat/rest-core/blob/rest-core-3.3.0/lib/rest-core/promise.rb#L14

[godfat]

@mrbrdo I probably see the point you're referring to. I would try to use SimpleDelegator or DelegateClass then, but I am not sure if it would work out. I'll try.

As for the kind_of? you're referring to, it's called on the hash of this:

https://github.com/godfat/rest-core/blob/rest-core-3.3.0/lib/rest-core/promise.rb#L47-L54

That means response[RESPONSE_BODY] and response[RESPONSE_HEADERS] and so on so forth are the futures/proxies we're talking about here. I guess the terms here are a bit confusing, and we could rename them to make things more clear.

This is how it is working: We create a Promise which would handle various stuffs, and generate a few Futures which wrap around RESPONSE_BODY, RESPONSE_HEADERS, and so on so forth. When someone is trying to evaluate response[RESPONSE_BODY] or response[RESPONSE_HEADERS], the Future would try to find the Promise and block there to wait for response, and then return the data.

[godfat]

Here's the line returning the hash (future_response) I was talking about:

https://github.com/godfat/rest-core/blob/rest-core-3.3.0/lib/rest-core/engine.rb#L12

[godfat]

So we could do this to switch to Delegate, however this breaks a lot of tests...
I am not sure if I should introduce this incompatibility.

diff --git a/lib/rest-core/promise.rb b/lib/rest-core/promise.rb
index c50e075..63e4b29 100644
--- a/lib/rest-core/promise.rb
+++ b/lib/rest-core/promise.rb
@@ -1,17 +1,18 @@

+require 'delegate'
 require 'thread'
 require 'rest-core'

 class RestCore::Promise
   include RestCore

-  class Future < BasicObject
+  class Future < Delegator
     def initialize promise, target
       @promise, @target = promise, target
     end

-    def method_missing msg, *args, &block
-      @promise.yield[@target].__send__(msg, *args, &block)
+    def __getobj__
+      @promise.yield[@target]
     end
   end