Guiceberry's JunitTestScope isn't thread-safe

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a guiceberry environment that binds some class (say, Foo) 
.in(TestScoped.class)
2. Create a test that starts several threads (say, with 
Executors.newFixedThreadPool(10))
3. Onto each thread, put a runnable that does:
   public void run() {
     Foo foo1 = fooProvider.get();
     try { Thread.sleep(1); } catch (InterruptedException e) {}
     Foo foo2 = fooProvider.get();
     assertSame(foo1, foo2);
   }
4. Arrange for all these threads to start simultaneously, and then somehow 
collect all the exceptions from the runnables.  (If any)

What is the expected output? What do you see instead?

I'd expect that within one test, something bound in @TestScoped could only 
have one instance.  Instead, one or two of these will come back with a 
different object, and the assertion will fail.

What version of the product are you using? On what operating system?
v2.0.1 on Ubuntu

Please provide any additional information below.

Original issue reported on code.google.com by dtm@google.com on 1 Dec 2009 at 6:54

[GoogleCodeExporter]

Patch that fixes this attached.

Original comment by dtm@google.com on 1 Dec 2009 at 7:03

Attachments:

• guiceberry_scope_patch.diff

[GoogleCodeExporter]

dtm,

I tried to reproduce your problem, and I couldn't -- see the inlined, fully 
spelled out (though 
inelegant) test. It passes. Maybe I did not understand your instructions, or 
maybe things are fine... 
Please send me a failing test for me to be able to take this further.

BTW, I also tried your patch, and it seems to have problems (several of my 
tests fail). I did not 
spent much time trying to figure out why (it might even have been a borked 
patch), which I guess will 
only be needed if there's indeed a problem.

**********************

/*
 * Copyright (C) 2008 Google Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package com.google.inject.testing.guiceberry.junit3;

import com.google.common.collect.Lists;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.testing.guiceberry.GuiceBerryEnv;
import com.google.inject.testing.guiceberry.TestScoped;

import junit.framework.TestCase;

import java.util.Collections;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

@GuiceBerryEnv(
    "com.google.inject.testing.guiceberry.junit3" + 
      ".ThreadSafetyTest" + 
        "$Env")
public class ThreadSafetyTest extends TestCase {

  @Inject
  private Provider<Foo> fooProvider;

  /**
   * See <a href="http://code.google.com/p/guiceberry/issues/detail?id=11">
   * Bug 11</a>.
   */
  public void testStressTestScopeThreadSafety() throws InterruptedException {
    ExecutorService executorService = Executors.newFixedThreadPool(10);

    final List<String> successes = 
      Collections.<String>synchronizedList(Lists.<String>newArrayList());
    final List<String> errors = 
      Collections.<String>synchronizedList(Lists.<String>newArrayList());
    Runnable runnable = new Runnable() {

       public void run() {
         Foo foo1 = fooProvider.get();
         try { 
           Thread.sleep(1); 
         } catch (InterruptedException e) {}
         Foo foo2 = fooProvider.get();
         if (foo1 == foo2) {
           successes.add("Success");
         } else {
           errors.add("Error");
         }
       }
    };
    for (int i=0; i< 10; i++) {
      executorService.execute(runnable);
    }
    while (errors.size() + successes.size() < 10) {
      Thread.sleep(10);
    }
    assertEquals(0, errors.size());
  }

  public static final class Foo {
    @Inject
    public Foo() {}
  }

  public static class Env extends GuiceBerryJunit3Env {

    @Override
    public void configure() {
      super.configure();
      bind(Foo.class).in(TestScoped.class);
    }
  }

  @Override
  protected void setUp() throws Exception {
    GuiceBerryJunit3.setUp(this);
  }
}

Original comment by zorze...@google.com on 1 Dec 2009 at 11:16

[GoogleCodeExporter]

Sorry, I was telling you the wrong place to put the delay to see the threading 
issues.

Also, yes, I misread the documentation on the return value of putIfAbsent, so 
my 
patch does have a mistake in it and will sometimes cause a NullPointerException.

Attached is a new patch, with a better fix and a test case based on your 
initial one.

I think it's possible to work something out so that the test case can reliably 
fail 
without the fixed scope without needing to have the delay in it.  Maybe 
something 
with a CyclicBarrier; I'll look into that.

Original comment by dtm@google.com on 2 Dec 2009 at 3:35

Attachments:

• guiceberry_scope_patch.diff

[GoogleCodeExporter]

And here's a better test that although it still needs a delay, can get by with a
relatively tiny one.  It also demonstrates the current issue pretty well:

/*
 * Copyright (C) 2009 Google Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package com.google.inject.testing.guiceberry.junit3;

import com.google.common.collect.Lists;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import com.google.inject.testing.guiceberry.GuiceBerryEnv;
import com.google.inject.testing.guiceberry.TestScoped;

import junit.framework.TestCase;

import java.util.Collections;
import java.util.List;
import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.Callable;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

@GuiceBerryEnv(
   "com.google.inject.testing.guiceberry.junit3" +
     ".ThreadSafetyTest" +
       "$Env")
public class ThreadSafetyTest extends TestCase {
  public static final int NTHREADS = 20;

  @Inject private Provider<Foo> fooProvider;
  @Inject private CyclicBarrier barrier;
  @Inject private AtomicInteger counter;

  /**
   * See <a href="http://code.google.com/p/guiceberry/issues/detail?id=11">
   * Bug 11</a>.
   */
  public void testStressTestScopeThreadSafety() throws Exception {
    ExecutorService executorService = Executors.newFixedThreadPool(NTHREADS);
    final List<String> successes =
        Collections.synchronizedList(Lists.<String>newArrayList());
    final List<String> errors =
        Collections.synchronizedList(Lists.<String>newArrayList());
    counter.set(0);

    Callable<?> runnable = new Callable<Void>() {
      public Void call() throws BrokenBarrierException, InterruptedException {
        barrier.await();
        Foo foo1 = fooProvider.get();
        barrier.await();
        Foo foo2 = fooProvider.get();
        if (foo1 == foo2) {
          successes.add("Success");
        } else {
          errors.add("Error: " + foo1 + " != " + foo2);
        }
        return null;
      }
    };
    for (int i=0; i< NTHREADS; i++) {
      executorService.submit(runnable);
    }
    executorService.shutdown();
    assertTrue("Didn't finish in 10 seconds",
        executorService.awaitTermination(10, TimeUnit.SECONDS));
    assertTrue("Foo is " + fooProvider.get() +
        "; Errors: " + errors, errors.isEmpty());
  }

  public static final class Foo {
    private final int ordinal;
    @Inject
    public Foo(AtomicInteger counter) throws InterruptedException {
      ordinal = counter.incrementAndGet();
      Thread.sleep(100);
    }
    @Override
    public String toString() {
      return "Foo[" + ordinal + "]";
    }
  }

  public static class Env extends GuiceBerryJunit3Env {

    @Override
    public void configure() {
      super.configure();
      bind(Foo.class).in(TestScoped.class);
      bind(CyclicBarrier.class).toInstance(new CyclicBarrier(NTHREADS));
      bind(AtomicInteger.class).in(Singleton.class);
    }
  }

  @Override
  protected void setUp() throws Exception {
    super.setUp();
    GuiceBerryJunit3.setUp(this);
  }
}

Original comment by dtm@google.com on 2 Dec 2009 at 1:30

[GoogleCodeExporter]

Excellent! Your code and test are very much appreciated! I have made one 
modification 
to the JUnitTestScope class to use the often-injustly bashed, tricky double 
checked 
lock, so as to avoid acquiring a mutex every time. I changed the HashMap to be 
a 
Concurrent one so that "get" has a predictable outcome... I think I got it 
right, but 
my I'm kind of tired, and I'll make sure to check again (since it *is* tricky).

Take a look for yourself, and tell me what you think.

Z

************************************

/*
 * Copyright (C) 2008 Google Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

//move to junit subpackage
package com.google.inject.testing.guiceberry.junit3;

import com.google.inject.Key;
import com.google.inject.Provider;
import com.google.inject.Scope;
import com.google.inject.Singleton;
import com.google.inject.testing.guiceberry.TestScoped;

import junit.framework.TestCase;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;

/**
 * The JUnit-specific implementation of the {@link TestScoped} annotation. 
 * 
 * @see GuiceBerryJunit3
 * @see Scope 
 *
 * @author Luiz-Otavio Zorzella
 * @author Danka Karwanska
 */
@Singleton
class JunitTestScope implements Scope {

  private final ConcurrentMap<TestCase, Map<Key<?>, Object>> testMap =
      new ConcurrentHashMap<TestCase, Map <Key<?>, Object>>();

  JunitTestScope() {}

  void finishScope(TestCase testCase) {
    testMap.remove(testCase); 
  }

  @SuppressWarnings("unchecked")  
  public synchronized <T> Provider<T> scope(final Key<T> key, 
      final Provider<T> creator) {

    return new Provider<T>() {
      public T get() {

        TestCase actualTestCase = GuiceBerryJunit3.getActualTestCase();
        if (actualTestCase == null) {
          throw new IllegalStateException(
              "GuiceBerry can't find out what is the currently-running test. " +
              "There are a few reasons why this can happen, but a likely one " +
              "is that a GuiceBerry Injector is being asked to instantiate a " +
              "class in a thread not created by your test case.");
        }
        Map<Key<?>, Object> keyToInstanceProvider = testMap.get(actualTestCase);
        if (keyToInstanceProvider == null) {
          testMap.putIfAbsent(
              actualTestCase, new ConcurrentHashMap<Key<?>, Object>());
          keyToInstanceProvider = testMap.get(actualTestCase);
        }
        Object o = keyToInstanceProvider.get(key);
        if (o != null) {
          return (T) o;
        }
        // double checked locking
        synchronized(keyToInstanceProvider) {
          o = keyToInstanceProvider.get(key);

          if (o == null) {
            o = creator.get();
            keyToInstanceProvider.put(key, o);

          }
          return (T) o;
        }
      }
    };
  }
}

Original comment by zorze...@google.com on 2 Dec 2009 at 10:30

[GoogleCodeExporter]

LGTM

Original comment by dtm@google.com on 2 Dec 2009 at 10:55

[GoogleCodeExporter]

Fixed on 2.0.2

Original comment by zorze...@gmail.com on 3 Dec 2009 at 12:53

• Changed state: Fixed