Change memoized method to not accept a block

[dkubb]

This branch changes the memoized method to not accept a block.

Originally in #6 we discussed using block_given? but it does not behave as expected within define_block. I changed it to yield |&block| which allows me to test block and raise an exception if a block was passed.

The only downside with this approach is that under 1.8.7 the new memoized method changes it's arity to 1. The method arity is correctly reported as 0 for 1.9.3+. I added a guard clause to skip this test under 1.8.7, but if anyone can think of an approach that allows the arity to remain 0 under 1.8.7 please let me know.

[dkubb]

Unless anyone has any suggestions or objections I will probably merge this within the next day or so.

[sferik]

My only thought for how to get a zero-arity method on Ruby 1.8.7 is to use instance_eval with block_given? instead of define_method. This would require some refactoring and is probably more pain that it’s worth but I think it’s the only way to solve this particular problem.

[mbj]

@dkubb ETA for release of adamantium with memoizable backend? I'd love to pull this behavior, as it is more strict and can possibly find coding errors.

[sferik]

If there are no objections, I can push a new version of the memoizable gem now.

[sferik]

By the way, I think this is a great example of why splitting off memoizable from adamantium was a good decision. I doubt this issue would have been raised and addressed as quickly if memoizable was not a separate project.

[dkubb]

@sferik go for it. Yeah, I much prefer smaller, single responsibility gems because the release cycles can be much quicker than large monolithic gems.

We're looking to extract other parts of the ROM ecosystem over the coming months too. Our convention is that if we make a supporting lib, we don't extract right away. Instead we put it in a support subdirectory (like https://github.com/dkubb/axiom-types/blob/master/lib/axiom/types/support/options.rb) and allow it to mature within the context of the project. Once it's reached a point where the churn is low and/or we have code in other gems that could use it, we extract it into a gem.

[sferik]

Cool. Releasing version 0.3.0 now. Giving it a new minor version (as opposed to a patch bump), just in case anyone depends on this behavior. Such users can safely depend on ~> 0.2.0.

[sferik]

Hmmm…I just pushed version 0.3.0. When I updated the twitter gem’s memoizable dependency, I started getting:

ThreadError:
  deadlock; recursive locking

This is literally the only change from the master branch, where tests are passing.

Here’s a link to the failing build: https://travis-ci.org/sferik/twitter/builds/15461976

Interestingly, I’m not seeing the deadlock on JRuby…

[dkubb]

@sferik interesting. I'll check it out tonight. Given that memoizable does nothing at all with Thread directly, I'm guessing this has exposed a bug in ThreadSafe::Cache, or there's some interaction between a dependent gem and thread_safe that is causing it.

[dkubb]

One of the changes I made since 0.2.0 is relying on ThreadSafe::Cache methods to perform cache updates in a more thread safe manner. I'm using the provided APIs though, not hand-rolling anything myself. I push down more work to the library rather than doing it in memoizable.

Worst case I could revert those changes, but I'd rather try to get to the bottom of it before making the decision to revert the changes.

[dkubb]

Forgive my rambling, I'm just leaving myself these notes as I think about them to aid in debugging later :)

The fact that the code works perfectly under JRuby leads me to believe that perhaps the ThreadSafe::Cache backend for the non-jruby implementations is buggy. The backend is chosen dynamically based on the ruby engine being used: https://github.com/headius/thread_safe/blob/master/lib/thread_safe/cache.rb#L10-L21 .. the SynchronizedCacheBackend is the fallback, and probably the safest one (and likely slowest). I wonder if we could force it to be used and see if it makes a difference. If the tests pass with it across all the rubies with this backend, then maybe it's a sign of a bug in MriCacheBackend and/or AtomicReferenceCacheBackend?

[dkubb]

Ok, so as a quick test I edited a local copy of ThreadSafe::Cache and set ConcurrentCacheBackend to NonConcurrentCacheBackend. When I ran the twitter tests everything passes.

[dkubb]

@sferik oh I see, you already hit these issues with https://github.com/headius/thread_safe/issues/15#issuecomment-25457042 ?

[dkubb]

@sferik How doe this look to you: https://github.com/dkubb/memoizable/pull/9

[sferik]

@dkubb Looks good to me. I just merged it. Thanks for tracking down that issue. I didn’t think that the root cause of this issue was the same issue I had with thread_safe when I was rolling my own Memoizable implementation inside the twitter gem.

We should be able to revert #9 if https://github.com/headius/thread_safe/issues/30 is ever resolved, but I’m not so optimistic that it will happen any time soon, so your patch is a good solution in the interim.