#13 Regression caused by new threadsafe API and defaults

[Daniel Gredler]

EasyMock 2.4 introduced a new threadsafe mode, but kept mocks non-threadsafe by default for performance reasons and due to "unknown side effects" [1].

The problem is that non-threadsafe mocks which are used by multiple threads now cause exceptions to be thrown, whereas in 2.3 and earlier there was no exception thrown. This change in default behavior will especially affect testing of Swing apps, which often need to deal with multiple threads (EDT, etc).

As things stand, the cost of upgrading to EasyMock 2.4 and going through all of our tests and changing the code to make all mocks threadsafe is prohibitive.

The possible fixes (as I see them) are:

1. Change the behavior of MocksBehavior#checkCurrentThreadSameAsLastThread() so that it just logs a warning, rather than throwing an exception.

2. Remove MocksBehavior#checkCurrentThreadSameAsLastThread(), so that non-threadsafe use matches version 2.3 and prior (no exception is thrown).

3. Make all mocks threadsafe by default.

4. Provide an easy way to set the default threadsafe value globally, rather than having to do it mock-by-mock or MocksControl-by-MocksControl.

Personally I think #3 is the best option, especially with the new JVMs that don't incur the synchronization cost unless necessary [2], but if that's completely out of the question, my order of preference after #3 would be #2, #1 and finally #4 (which would just barely be acceptable).

Looking forward to feedback,

Daniel

[1] http://tech.groups.yahoo.com/group/easymock/message/1205

[2] http://java.sun.com/performance/reference/whitepapers/6_performance.html#2.1

[Henri Tremblay]

Hi,

I was aware of this change of behavior when I did it. I had two choices.

1) Make everything thread safe
2) Allow the user to make a mock thread safe and so making him know the real object should be too

I picked 2. I assumed that changing the code to be compliant won't be that hard. Just to add a makeThreadSafe where needed.

The other options was to be thread-safe by default and allow people to check for thread safety. The problem with this is that it won't force people to think about thread safety. How many tests to you have so that it would take so long?

Because right now #4 seems to be my favorite.

[Daniel Gredler]

Hi Henri,

Thanks for the quick feedback. I can understand the "tough love" approach, it's just a bit too tough for us right now :-) A quick search tells me we have about 5,000 unit tests in our project. Half of these (about 2,500) are clientside, meaning they're the ones that are probably vulnerable to this regression. However, I don't know exactly how many of these are affected, because a) I would have to aggregate the test failure counts from 20+ modules and b) the regression doesn't cause the tests to fail -- it causes them to freeze.

Considering your goal of making users think about the threadsafety of the classes which they are mocking, and your preference for option 4, perhaps a good compromise would be an API for setting global per-class defaults... something like EasyMock.registerAsThreadSafe(Foo.class, true)... or something like that.

But I still like the other options better :-)

Take care,

Daniel

[Henri Tremblay]

I gave is some thoughts. I think the best solution is to add an easymock.properties file to the classpath allowing to specified thread-safe rule shouldn't be enforced. That should solve your issue.

What do you think?

[DnD]

We had a problem migrating to easymock 2.4...I gave up and stayed with 2.3, because the tests would not work in thread-safe or non-threadsafe mode. Our tests would deadlock in thread-safe mode, or throw the thread safety exception.

I can't remember exactly now, but I think it was because EasyMock's 'thread-safe' behaviour forces synchronization that does not necessarily exist in the mocked class, even if that class is thread-safe. The mocked object is synchronized on the entire object, which is more coarse-grained than most concurrent classes.

Easy example I can think of is trying to mock a ConcurrentMap - if you use Answer objects etc that allow you to force eg a get() to wait for a put() to be invoked (or vice versa), then it will just deadlock because the get() will lock the mock, so the put() will never be able to proceed. This would obviously not happen in a real ConcurrentMap, as the locking much more finely grained.

[Henri Tremblay]

I'm looking at the deadlock issue. I can't find why it would deadlock. If the get block the put, when the put ends, the get will go through.

Can you send me a thread dump of an EasyMock thread-locking issue? I would like to know if EasyMock is deadlocking with itself or because of the synchronization of tested objects (which is the more likely).

I'll also check if I can have a thread-safe none blocking behavior.

[Henri Tremblay]

The final solution is:

• Mocks are now thread-safe by default
• Mocks are not checked for thread-safety by default anymore

Plus two properties were added to change that default.

It is also possible to call makeThreadSafe and checkIsUsedInOneThread to change those behavior on a given mock.

So I agree that the 2.4 behavior wasn't a good idea but now there should be enough flexibility to do anything. However, it's not possible to have EasyMock not being blocking. Every actual needs to be remembered.

[DnD]

sorry, i have not been paying attention. basically, if you want to mock a get() call that must wait until a put() call has been invoked, then it would deadlock because the get() call would lock the mock object while it was waiting. so the put() would be waiting for the get() to finish before being invoked, and the get() would be waiting for the put() to be invoked.

i'll try out 2.5.1 and see how it goes.