Allow @TestScoped to play nicely with a shared thread pool

[timothybasanov]

Resolves: https://github.com/zorzella/guiceberry/issues/35

Changes TestScope contract to explicitly propagate to children threads and to be explicitly closed when test execution is finished. This prevents several threads from accidentally sharing the same ThreadScope by reusing a thread pool.

Add an additional API to allow TestScope propagation to non-children threads and shared thread pools: InternalState TestScope.getInternalStateForCurrentThread(); void TestScope.setInternalStateForCurrentThread(InternalState);

This allows testing of multi-threaded servers that have a static internal thread pool shared between tests.

[yuriks]

Hi, I had the same issue a few weeks ago and didn't think of looking at the PR list before I started implementing my own fix. Oops.

I have a slightly different API to do the scope propagation, but your implementation seems to more thought out overall, especially with making this issue easier to detect (and not require me to spend a day with a debugger figuring out what was going on.)

Just in case you're interested, this is what I was planning on exposing: https://github.com/yuriks/guiceberry/commit/0a59cc3ac42ec92e462016641a4af22b41302ed9 I then wrote a Executor wrapper which wraps this around the Runnable before sending it to the thread pool. Worked fine and seemed pretty reasonable in terms of cleanliness.

Either way, I'll be eagerly watching this. :)

[zorzella]

Sorry for the delay. This got to me when I was particularly busy, and then got buried in other email.

If I understand this correctly, this PR deals with the following scenario:

a. Foo is bound in a @TestScope b. MyTestClass has (directly or indirectly) access to a "@Inject Provider fooProvider" field c. TestOne in MyTestClass (directly or indirectly) spawns WorkerThreadOne, and stores it in some static field d. The test sends as work to WorkerThreadOne a method wrappedGetFoo() that calls "fooProvider.get()" and returns that value. Since ThreadOne is a child of the test's thread (and given that @TestScopes are inheriting thread locals), that will work properly, and: i. WorkerThreadOne will have an instance of Foo to return; and ii. TestScope will cache that instance of "Foo" (as it's its job) to be given out to this test if (say) "fooProvider.get()" is called again (from wrappedGetFoo() or elsewhere) e. TestOne finishes f. TestTwo accesses the statically-stored WorkerThreadOne, and again sends wrappedGetFoo() as work to that thread g. The Foo returned by this call to wrappedGetFoo() will be the instance that was created in step #d -- i.e. by the former TestOne, which is surprising, given that Foo is @TestScoped

Ok, so your PR does two things:

1. It causes #f to fail if it happens after “e” (i.e. if TestOne has already finished)
2. It creates a new API to allow one to cause TestScope to behave as though WorkerThreadOne had been restarted in TestTwo

Furthermore, you say that “this allows testing of multi-threaded servers that have a static internal thread pool shared between tests.”

I’m trying to infer from that when this situation actually occurs in the wild. What I’m guessing is that:

A) You are using the GuiceBerry injector to create instances of your server under test. You’re probably doing this by having the GuiceBerry injector modules include several (probably most) of your prod server modules; and B) That you somehow overriding some of these prod bindings to have them be @TestScoped

Ok, so let’s go by parts:

Doing “A” is discouraged and non-idiomatic. GuiceBerry tests will generally have two injectors -- the test injector (created by the GuiceBerry harness) and the injector used to create your server-under-test (which is, again, idiomatically, created in a GuiceBerryEnvMain). Yes, I’ve seen people do this on occasion, and, if you really know what you are doing, it’s not all that terrible. That said, I’ve never seen a scenario where this wouldn’t be better dealt with in the idiomatic fashion. At the most, it sometimes happens that it’s really compelling to be able to inject some of the things created by your server-under-test injector into your test, and, for that, there’s a pattern to expose those. Even doing this is advanced, and you should really know what you are doing to not shoot yourself in the foot…

Now, doing “B” feel unequivocally bad, and I’m not particularly surprised that things don’t work as expected -- they can’t work as expected, because there are no sane expectations for one to have. To wit, let’s say we submit your PR. And then let’s say we run TestOne and TestTwo in parallel with each other. Depending on how the tests are written, you’d possibly end up with one of (at least) four outcomes: TestTwo getting the Foo instance created by TestOne (since TestOne isn’t necessarily “finished” while TestTwo is running); or you’d get an exception saying that TestOne has finished; or you can have TestOne get the instance created by TestTwo (if that happens after TestTwo calls setInternalStateForCurrentThread()); or you get what you hope.

I would much rather try to understand if there is any particular reason why you feel you must do “A” and “B”, and how to address that.

BTW, something along the lines of “1” might be an interesting addition, just to help people identify when they are doing something bad, though the suggested fix would be something along the lines of “stop doing [describe A] and [describe B]”.

Let me know your thoughts,

Z

[timothybasanov]

Thanks for the full response, Z! I'm so excited!

You're right, that I completely forgot to explain how have I got into this situation. I need to fully explain my situation and what was I trying to achieve and why and how did it fail for me. I do not think I've done something that would be considered a bad practice for Guice, but I'll be happy to find out that I just used the Guice wrong. :)

Ok. Let me first describe what am I doing in my tests:

• I have a suite of "functional" tests that share the same Guiceberry environment
  • Environment does not install any other modules, all configuration is done only via a set of "provides" methods
  • Env includes a test RPC server with a simple fake implementation
  • I use only one GuiceBerry-created injector shared across all tests
  • RPC server is in Singleton scope to share it across tests
  • All tests are usually run sequentially (they do support running in parallel, but usually I don't do it)
• RPC server uses "in-process" channels and a fake implementation that (simplified) prints out a current TestId
  • RPC server is created in Guiceberry env and uses anonymous classes to fit the whole server implementation within a single "provides" method
  • Due to RPC server internal implementation details it's not possible to execute requests on a calling thread and an internal thread pool is required
  • This thread pool is shared across all tests and is lazily created during the very first RPC request during the first test
  • As the whole server implementation fits within a "provides" method I injected a test-scoped TestId provider as a method param and logged it within an RPC server fake implementation

Roughly speaking:

public class GuiceBerryMinimalFunctionalTest {
  interface RpcServer {
    String serverCall() throws Exception;
  }

  public static class Environment extends GuiceBerryModule {
    public Environment() { }

    @Provides @Singleton RpcServer createRpcServer(Provider<TestId> testIdProvider) {
      ExecutorService executorService = Executors.newCachedThreadPool();
      return new RpcServer() {
        @Override public String serverCall() throws Exception {
          // FIX: internalState = testScope.getInternalStateForCurrentThread();
          return executorService.submit(new Callable<String>() {
            @Override public String call() throws Exception {
              // FIX: testScope.setInternalStateForCurrentThread(internalState);
              return testIdProvider.get().toString();
            }
          }).get();
        }
      };
    }
  }

  public @Rule GuiceBerryRule guiceBerryRule = new GuiceBerryRule(Environment.class);
  @Inject RpcServer rpcServer;
  @Inject Provider<TestId> testIdProvider;

  @Test public void test1() throws Exception {
    Assert.assertEquals(testIdProvider.get().toString(),
        rpcServer.serverCall());
  }

  @Test public void test2() throws Exception {
    // TODO: Fix this test by applying a FIX above
    Assert.assertEquals(testIdProvider.get().toString(),
        rpcServer.serverCall());
  }
}

Expected: Current TestId is returned during test's execution when RPC server is called or GuiceBerry throws exceptions with explanation why TestScope does not work here Actual: The very first test works as expected, subsequent tests may fail Note: Actual implementation was much more involved. It was not TestId that broke my tests. It was extremely hard to diagnose and debug. I have not expected TestScope to effectively "cache" results in a propagating thread-local

What do I want to achieve with my change:

• Try really hard to not to break an "expected" contract of TestScope. My change should not break existing tests that use GuiceBerry
• Allow to detect such a condition when all tests are run sequentially. Do not break parallel tests runner. I accept that when tests are run in parallel I'll not be able to detect the issue in some cases
• Provide useful message and debugging information. The crucial part is the name for the "second" TestId and a current thread name where instance was already created
• Provide some API that would allow people to implement scope propagation. Use syntax similar to LogContext or TraceContext scope propagation. Provide this API discoverability in the exception message

I think I have addressed your comment, but I probably have missed something. Also, not sure if I understand how to properly use two injectors for tests... so I'm probably missing something very important about Guiceberry. If you can give me a link to some documentation it would be useful.

Let me know what you're thinking. I'm ready to work on my PR to satisfy any of yours requirements. :)

[zorzella]

Hey, to sort things out, it feels to me very important that you understand that a canonical GuiceBerry test will involve two injectors. E.g. I quote from my doc: https://docs.google.com/document/d/1Cyism50KXTMXcvYNbqUXi3gaaNKaXDgjXjpx4rPNet8/preview

"First, note that there are two Injectors -- one created by GuiceBerry, which is used to @Inject the test; and the other created in the constructor of your server. This is, as we point out elsewhere, the canonical way to use GuiceBerry, and is the most important thing I need you to understand in this example. Do not get confused by them. Note that you can't "@Inject @PortNumber int" on the Servlet, just like you can't "@Inject @Featured Pet" in the test."

I recommend you watch my talk that walks through the tutorial https://www.youtube.com/watch?v=yqre07YfPXQ and pay attention to where I talk about the "server injector" vs the "test injector" (e.g. around 22:50).

What is still not clear to me from your more extensive example is what you consider to be the code under test vs what is support code. More specifically, you say you have "a test RPC server with a simple fake implementation", and then you have this assertion:

Assert.assertEquals(testIdProvider.get().toString(), rpcServer.serverCall());

I understand this is a simplification, but n.b. several weird things:

1. if this is a fake implementation, why are you asserting its results

2. an implementation that involves a threadpool can hardly be considered simple

What I imagine you are doing is:

a. you have a server that you want to test, which you did not include in this simplification. Let's call this server FooServer

b. That server needs to talk a backend

c. You want to have your test tweak what the backend returns to that server, so you can exercise some code.

d. So you decided to create this so-called simple server that implement RpcServer

Again, this is just my guess...

BTW, for the code as written above, even if you preserve your test structure exactly as is, I think there's a pretty simple fix to the problem, which would be:

return new RpcServer() { @Override public String serverCall() throws Exception { TestId myTestId = testIdProvider.get(); // FIX: internalState = testScope.getInternalStateForCurrentThread(); return executorService.submit(new Callable() { @Override public String call() throws Exception { // FIX: testScope.setInternalStateForCurrentThread(internalState); return myTestId; } }).get();

I don't necessarily think this is a good solution to your real problem (which I'm still guessing about), but it sure would work for your example, and it's much simpler than the whole "scope propagation" dance. The question of a better error message to avoid you wasting your time chasing this bug is addressed below.

I'll also respond here to your bullet points:

---

• Try really hard to not to break an "expected" contract of TestScope. My change should not break existing tests that use GuiceBerry

The PR as is does not break any existing test, but: a. would create the conditions for even more surprising outcomes; b. would make public a class that is currently an internal implementation detail; c. add a complex and dangerous API in a visible location

• Allow to detect such a condition when all tests are run sequentially. Do not break parallel tests runner. I accept that when tests are run in parallel I'll not be able to detect the issue in some cases. Provide useful message and debugging information. The crucial part is the name for the "second" TestId and a current thread name where instance was already created

I do see some value in helping people figure out what's wrong. Even if this can't work for parallel test execution, it would be valuable for most test invocations. That's what I meant when I said: BTW, something along the lines of “1” might be an interesting addition, just to help people identify when they are doing something bad, though the suggested fix would be something along the lines of “stop doing [describe A] and [describe B]”. (btw, I just noticed that github mangled my last reply, and I fixed it there -- you might want to re-read it as it is hopefully less cryptic)

• Provide some API that would allow people to implement scope propagation. Use syntax similar to LogContext or TraceContext scope propagation. Provide this API discoverability in the exception message

That's the part I'm skeptical about. I hope you can be satisfied there is a fundamentally better way of doing what you want.

[zorzella]

I never got a reply to my questions, so I'll just close this PR.