Promise extends IVar

[jdantonio]

Promise extends IVar

Promise extends IVar the way Future does. This allows a Promise to be manually (and safely) be set from any thread. It also begins the process of consolidating Promise, Future, and IVar.

See #257, #139, and #269

IVar is a Synchronization::Object

IVar is now a Synchronization::Object.

• Dereferenceable supports injection of a mutex
• Obligation supports injection of a mutex
• IVar extends Synchronization::Object
• IVar passes self to Obligation as injected mutex
• IVar, Promise, and Future no longer call mutex#lock

See #273 and #275

Moved mutex unlocking into ensure clauses.

See #244

[pitr-ch]

Thanks @jdantonio, looks much better then my PR. Testing.

[pitr-ch]

It might make sense to also call execute here.

[obrok]

Sorry if this doesn't make sense, because I might not have the full context:

I run into a problem with manually setting promises where if you attach child promises execute will sometimes get called on your promise before you manage to set its value, triggered by one of the children. My workaround for that was the following:

class FulfillablePromise < Concurrent::Promise
  def initialize(*)
    super
    @state = :pending
  end

  def fulfill(value)
    on_fulfill(value)
  end

  def reject(error)
    on_reject(error)
  end
end

[jdantonio]

@obrok That is very good information. Thank you. I think the context of this PR may be a little different than your example. If I correctly understand your example, it is possible to manually set the value of a child Promise. In this use case we only want to manually set the root promise and never manually set the children. Basically:

• Create the root Promise but do not execute
• Add children to form a tree/chain
• Manually set or fail the root Promise
• Allow the chain/tree to execute normally based on the new state of the root Promise

There may be bugs in this implementation because it is just a spike, but the intent is that we would never manually set any of the children. This is why set and fail raise exceptions when not the root Promise.

Does that make sense, or do I misunderstand your example?

[obrok]

It makes sense but I still think there might be a problem. Consider this function (which I imagine is the use case):

def do_something_later
  promise = Promise.new
  Thread.new { promise.set(:result) }
  promise
end

do_something_later.then { |result| p result }.execute
# I think this will output nil most of the time or possibly it's a race

The client of do_something_later is well within his rights to add children to it and possibly call execute on the children. If the root promise is in the :unscheduled state it will cause it to execute the default empty block and set its value to nil.

[jdantonio]

@pitr-ch This version of set follows your suggestion and calls execute. The function itself could use a little cleanup but the tests still pass. I think the behavior here is probably more accurate. I haven't changed fail yet but I anticipate it will be very similar except for something like this:

@promise_body = Proc.new { |result| raise reason }

Thoughts?

[jdantonio]

@obrok That makes sense. Once we get the behavior right for manually setting the root promise we can dig more deeply into your example.

[pitr-ch]

Based on the tests it seems there is a race hidden. What is the purpose of NO_VALUE? I am still testing with Dynflow, looks good but I am hitting another issue with pools - investigating. We could also consider running Dynflow test suite within concurrent-ruby travis tests (and other projects we can find) to test some real usage scenario.

[jdantonio]

NO_VALUE was part of @chrisseaton's original implementation of IVar and we've always carried it forward.

@chrisseaton Can you answer @pitr-ch's question, please?

[chrisseaton]

The point of NO_VALUE was to represent that the IVar currently had no value. I didn't want to use nil as I wanted nil to be a valid value for an assigned IVar. However a better option would be to use an array to hold the value, so an empty array for no value, or to have a flag to say if there is a value or not.

[jdantonio]

This update makes Delay a little slower. We may want to rollback the most recent commit and reset Delay. Performance from that class is probably more important than making it a Synchronization::Object.

BEFORE

$ ./examples/benchmark_lazy_and_delay.rb
Rehearsal -------------------------------------------------------
Delay#value           0.360000   0.000000   0.360000 (  0.361039)
Delay#value!          0.430000   0.000000   0.430000 (  0.438568)
LazyReference#value   0.290000   0.000000   0.290000 (  0.285422)
---------------------------------------------- total: 1.080000sec

                          user     system      total        real
Delay#value           0.370000   0.000000   0.370000 (  0.376526)
Delay#value!          0.470000   0.000000   0.470000 (  0.470254)
LazyReference#value   0.270000   0.000000   0.270000 (  0.268231)

AFTER

$ ./examples/benchmark_lazy_and_delay.rb
Rehearsal -------------------------------------------------------
Delay#value           0.440000   0.000000   0.440000 (  0.448287)
Delay#value!          0.560000   0.000000   0.560000 (  0.563817)
LazyReference#value   0.280000   0.000000   0.280000 (  0.282739)
---------------------------------------------- total: 1.280000sec

                          user     system      total        real
Delay#value           0.450000   0.000000   0.450000 (  0.461807)
Delay#value!          0.540000   0.000000   0.540000 (  0.552702)
LazyReference#value   0.270000   0.000000   0.270000 (  0.273209)

[pitr-ch]

@jdantonio I would suggest merging with Synchronization::Object. The correctness (ivar read is not volatile on Rubinius) is more important than slight performance gain. I also think we will get it back when volatile reads and final-field-constructor-visibility is abstracted in Synchronization to be able to remove synchronization around lot of the ivar reads which I think is source of the performance decrease.

[jdantonio]

@pitr-ch Sounds good. I'll keep it as-is. I'll rebase this on master once you merge #274.

[jdantonio]

The test that failed in the last run is one that is known to fail intermittently. See #271

[jdantonio]

@pitr-ch Is it OK with you if I merge this? Or would you rather I wait until #274 is ready?

[pitr-ch]

Since delay is migrated to Delay < Synchronization::Object it probably should be using its method synchronize, is there a reason for using mutex which I am missing?

[jdantonio]

It is using the #synchronize method from Synchronization::Object, it's just doing so through a level of indirection. See line 64. Delay includes Obligation which also includes Dereferenceable, the latter of which creates its own mutex within the #init_mutex function. I've added an argument to both #init_obligation and #init_mutex that allows the mutex to be injected. Delay passes self into those initialization functions, thus allowing the delay object to be its own lock. The #mutex method returns self. The mutex.synchronize call on line 150 is actually self.synchronize.

[pitr-ch]

Ah right, I've missed that. It seems unnecessarily complicated, but I am not sure if it makes sense to invest time in refactoring it - I still hope that the new futures will replace this class soon.

[pitr-ch]

@jdantonio I've looked through the code and added few comments. I think this is not dependent on #274. This brings some of the features of the new futures to the original implementation. (I really have to sit down soon and write a documentation for that.)