EM::Synchrony.sync behavior #117

Closed
sujal opened this Issue Mar 14, 2012 · 7 comments

Projects

None yet

3 participants

sujal commented Mar 14, 2012

Hey folks,

I'm looking into those testing errors that Josh found with my mongoid work and just ran across something odd. here's the definition of EM::Synchrony.sync

def self.sync(df)
  f = Fiber.current
  xback = proc do |*args|
    if f == Fiber.current
      return args.one? ? args.first : args
    else
      f.resume(*args)
    end
  end

  df.callback &xback
  df.errback &xback

  Fiber.yield
end

I have a method that returns nil out of the deferrable to the successful callback. The code above:

return args.one? ? args.first : args

is causing the sync call to return [nil] instead of nil. This makes sense because the definition of one? is that an element as to return true from the collection. Nil obviously won't.

So... question is this: is this side effect of one? an intentional thing? Or would we be better off switching #sync to use #size instead?

I've worked around this in the particular spot it was biting me, but I would love to get it "right" whatever that means. :)

Sujal

Contributor
lgierth commented Mar 14, 2012

IMO it's not intentional. Passing nil or false to #succeed/#fail should be a perfectly valid use-case.

1.9.2p318 :001 > [nil].length
 => 1 
1.9.2p318 :002 > [nil].one?
 => false

So let's go with return args.length <= 1 ? args[0] : args, WDYT @igrigorik ?

Owner

@lgierth yep, almost. I think we want to check for == not <=. If the args array is empty, it would have returned an empty array (previously), which I'm thinking we want to preserve..

Contributor
lgierth commented Mar 14, 2012

So the argument/result map would look like this:

{
  [] => [],
  [1] => 1,
  [1, 2] => [1, 2]
}

Get my point? :trollface:

Owner

Wait a second.. I'm trying to understand why I even implemented this way to begin with.. :-)

   f = Fiber.current
      xback = proc do |*args|
        if f == Fiber.current
          return *args
        else
          f.resume(*args)
        end
      end

What's wrong with that?

Contributor
lgierth commented Mar 15, 2012

Dunno, but apparently something needed to get "fixed" :) The old return without conditional seems to be the most consistent and predictable approach.

Owner

Well, since we're looking at the code, I'm leaning towards the return *args.. I'm not sure why we had the special case before.

sujal commented Mar 16, 2012

return *args is definitely simpler. I don't have a strong opinion on this. It just felt odd, which is the only reason I brought it up. For a moment, I was hoping that was why tests wouldn't run on the mongoid branch, but alas, had nothing to do with it. Still chasing that particular annoyance down.

@igrigorik igrigorik closed this in 323c75b Mar 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment