Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for ManagedThreadFactory Injected to Servlet #423

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

aubi
Copy link
Contributor

@aubi aubi commented Feb 5, 2024

No description provided.

@aubi aubi changed the title Add tests for ManagedThreadfactory Add tests for ManagedThreadFactory Injected to Servlet Feb 5, 2024
Comment on lines +104 to +107
assertEquals(results.poll(MAX_WAIT_SECONDS, TimeUnit.SECONDS), 161,
"Third-party context type IntContext must be propagated to thread "
+ "per ManagedThreadFactoryDefinition and ContextServiceDefinition configuration "
+ "based on the thread context at the time the ManagedThreadFactory was looked up.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asserted value and message are contradictory. The value when looked up is 0 (default).

Suggested change
assertEquals(results.poll(MAX_WAIT_SECONDS, TimeUnit.SECONDS), 161,
"Third-party context type IntContext must be propagated to thread "
+ "per ManagedThreadFactoryDefinition and ContextServiceDefinition configuration "
+ "based on the thread context at the time the ManagedThreadFactory was looked up.");
assertEquals(results.poll(MAX_WAIT_SECONDS, TimeUnit.SECONDS), 0,
"Third-party context type IntContext must be propagated to thread "
+ "per ManagedThreadFactoryDefinition and ContextServiceDefinition configuration "
+ "based on the thread context at the time the ManagedThreadFactory was looked up.");

"Transaction context must be cleared from async Callable task "
+ "per java:comp/concurrent/ThreadFactoryInjB configuration.");
} finally {
IntContext.set(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntContext is not used in this test. It should not be necessary to reset it.

Suggested change
IntContext.set(0);

Comment on lines +200 to +227
IntContext.set(2000);
StringContext.set("testParallelStreamBackedByManagedThreadFactory-2");

fj = new ForkJoinPool(4, threadFactoryInjA, null, false);

IntContext.set(3000);
StringContext.set("testParallelStreamBackedByManagedThreadFactory-3");

ForkJoinTask<Optional<Integer>> task = fj.submit(() -> {
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9).parallelStream().map(num -> {
assertEquals(StringContext.get(), "",
"Third-party context type StringContext must be cleared on " + "ForkJoin thread.");
try {
assertNotNull(InitialContext.doLookup("java:app/concurrent/ContextA"),
"Application context must be propagated to ForkJoin thread");
} catch (NamingException x) {
throw new CompletionException(x);
}
return num * Thread.currentThread().getPriority() + IntContext.get();
}).reduce(Integer::sum);
});

Optional<Integer> result = task.join();
assertEquals(result.get(), Integer.valueOf(9180),
"Third-party context type IntContext must propagated to ForkJoin threads "
+ "(thousands digit should be 9) and thread priority (4) must be enforced "
+ "on ForkJoin threads (hundreds/tens/ones digits must be 4x5x9=180) "
+ "per configuration of the ManagedThreadFactoryDefinition and ContextServiceDefinition.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a copy/paste error from the other test that had IntContext=1000 on the thread when the lookup was performed. For this one, the IntContext is 0 when looked up, and needs to be updated as follows:

Suggested change
IntContext.set(2000);
StringContext.set("testParallelStreamBackedByManagedThreadFactory-2");
fj = new ForkJoinPool(4, threadFactoryInjA, null, false);
IntContext.set(3000);
StringContext.set("testParallelStreamBackedByManagedThreadFactory-3");
ForkJoinTask<Optional<Integer>> task = fj.submit(() -> {
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9).parallelStream().map(num -> {
assertEquals(StringContext.get(), "",
"Third-party context type StringContext must be cleared on " + "ForkJoin thread.");
try {
assertNotNull(InitialContext.doLookup("java:app/concurrent/ContextA"),
"Application context must be propagated to ForkJoin thread");
} catch (NamingException x) {
throw new CompletionException(x);
}
return num * Thread.currentThread().getPriority() + IntContext.get();
}).reduce(Integer::sum);
});
Optional<Integer> result = task.join();
assertEquals(result.get(), Integer.valueOf(9180),
"Third-party context type IntContext must propagated to ForkJoin threads "
+ "(thousands digit should be 9) and thread priority (4) must be enforced "
+ "on ForkJoin threads (hundreds/tens/ones digits must be 4x5x9=180) "
+ "per configuration of the ManagedThreadFactoryDefinition and ContextServiceDefinition.");
IntContext.set(2000);
StringContext.set("testParallelStreamBackedByManagedThreadFactory-2");
fj = new ForkJoinPool(4, threadFactoryInjA, null, false);
IntContext.set(3000);
StringContext.set("testParallelStreamBackedByManagedThreadFactory-3");
ForkJoinTask<Optional<Integer>> task = fj.submit(() -> {
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9).parallelStream().map(num -> {
assertEquals(StringContext.get(), "",
"Third-party context type StringContext must be cleared on " + "ForkJoin thread.");
try {
assertNotNull(InitialContext.doLookup("java:app/concurrent/ContextA"),
"Application context must be propagated to ForkJoin thread");
} catch (NamingException x) {
throw new CompletionException(x);
}
return num * Thread.currentThread().getPriority() + IntContext.get();
}).reduce(Integer::sum);
});
Optional<Integer> result = task.join();
assertEquals(result.get(), Integer.valueOf(180),
"Third-party context type IntContext must be propagated to ForkJoin threads "
+ "(thousands digit should be 0) and thread priority (4) must be enforced "
+ "on ForkJoin threads (hundreds/tens/ones digits must be 4x5x9=180) "
+ "per configuration of the ManagedThreadFactoryDefinition and ContextServiceDefinition.");

Comment on lines +117 to +120
assertEquals(results.poll(MAX_WAIT_SECONDS, TimeUnit.SECONDS), Integer.valueOf(161),
"Third-party context type IntContext must be propagated to thread "
+ "per ManagedThreadFactoryDefinition and ContextServiceDefinition configuration "
+ "based on the thread context at the time the ManagedThreadFactory was looked up.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntContext is 0 (default) when looked up.

Suggested change
assertEquals(results.poll(MAX_WAIT_SECONDS, TimeUnit.SECONDS), Integer.valueOf(161),
"Third-party context type IntContext must be propagated to thread "
+ "per ManagedThreadFactoryDefinition and ContextServiceDefinition configuration "
+ "based on the thread context at the time the ManagedThreadFactory was looked up.");
assertEquals(results.poll(MAX_WAIT_SECONDS, TimeUnit.SECONDS), Integer.valueOf(0),
"Third-party context type IntContext must be propagated to thread "
+ "per ManagedThreadFactoryDefinition and ContextServiceDefinition configuration "
+ "based on the thread context at the time the ManagedThreadFactory was looked up.");

try {
allThreadsRunning.countDown();
blocker.await(MAX_WAIT_SECONDS * 5, TimeUnit.SECONDS);
lookupTaskResult.complete(threadFactoryB);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Runnable named "lookupTask" wasn't actually performing a lookup, just returning an existing value, which didn't test context propagation at all.

Suggested change
lookupTaskResult.complete(threadFactoryB);
lookupTaskResult.complete(InitialContext.doLookup("java:comp/concurrent/EJBThreadFactoryB"));

assertTrue(result instanceof ManagedThreadFactory, "Application context must be propagated to first thread "
+ "per java:comp/concurrent/EJBThreadFactoryB configuration.");
} finally {
IntContext.set(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this if you want to. The test doesn't use IntContext so it shouldn't need to be cleared.

Suggested change
IntContext.set(0);
IntContext.set(0);

Comment on lines +212 to +239
IntContext.set(2000);
StringContext.set("testParallelStreamBackedByManagedThreadFactoryEJB-2");

fj = new ForkJoinPool(4, threadFactoryA, null, false);

IntContext.set(3000);
StringContext.set("testParallelStreamBackedByManagedThreadFactoryEJB-3");

ForkJoinTask<Optional<Integer>> task = fj.submit(() -> {
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9).parallelStream().map(num -> {
assertEquals(StringContext.get(), "",
"Third-party context type StringContext must be cleared on " + "ForkJoin thread.");
try {
assertNotNull(InitialContext.doLookup("java:app/concurrent/ContextA"),
"Application context must be propagated to ForkJoin thread");
} catch (NamingException x) {
throw new CompletionException(x);
}
return num * Thread.currentThread().getPriority() + IntContext.get();
}).reduce(Integer::sum);
});

Optional<Integer> result = task.join();
assertEquals(result.get(), Integer.valueOf(9180),
"Third-party context type IntContext must propagated to ForkJoin threads "
+ "(thousands digit should be 9) and thread priority (4) must be enforced "
+ "on ForkJoin threads (hundreds/tens/ones digits must be 4x5x9=180) "
+ "per configuration of the ManagedThreadFactoryDefinition and ContextServiceDefinition.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as other comment - this looks like a copy/paste error from a test where IntContext=1000 was on the thread during lookup. In this case, IntContext is 0 (defaulted) during lookup.

Suggested change
IntContext.set(2000);
StringContext.set("testParallelStreamBackedByManagedThreadFactoryEJB-2");
fj = new ForkJoinPool(4, threadFactoryA, null, false);
IntContext.set(3000);
StringContext.set("testParallelStreamBackedByManagedThreadFactoryEJB-3");
ForkJoinTask<Optional<Integer>> task = fj.submit(() -> {
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9).parallelStream().map(num -> {
assertEquals(StringContext.get(), "",
"Third-party context type StringContext must be cleared on " + "ForkJoin thread.");
try {
assertNotNull(InitialContext.doLookup("java:app/concurrent/ContextA"),
"Application context must be propagated to ForkJoin thread");
} catch (NamingException x) {
throw new CompletionException(x);
}
return num * Thread.currentThread().getPriority() + IntContext.get();
}).reduce(Integer::sum);
});
Optional<Integer> result = task.join();
assertEquals(result.get(), Integer.valueOf(9180),
"Third-party context type IntContext must propagated to ForkJoin threads "
+ "(thousands digit should be 9) and thread priority (4) must be enforced "
+ "on ForkJoin threads (hundreds/tens/ones digits must be 4x5x9=180) "
+ "per configuration of the ManagedThreadFactoryDefinition and ContextServiceDefinition.");
IntContext.set(2000);
StringContext.set("testParallelStreamBackedByManagedThreadFactoryEJB-2");
fj = new ForkJoinPool(4, threadFactoryA, null, false);
IntContext.set(3000);
StringContext.set("testParallelStreamBackedByManagedThreadFactoryEJB-3");
ForkJoinTask<Optional<Integer>> task = fj.submit(() -> {
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9).parallelStream().map(num -> {
assertEquals(StringContext.get(), "",
"Third-party context type StringContext must be cleared on " + "ForkJoin thread.");
try {
assertNotNull(InitialContext.doLookup("java:app/concurrent/ContextA"),
"Application context must be propagated to ForkJoin thread");
} catch (NamingException x) {
throw new CompletionException(x);
}
return num * Thread.currentThread().getPriority() + IntContext.get();
}).reduce(Integer::sum);
});
Optional<Integer> result = task.join();
assertEquals(result.get(), Integer.valueOf(180),
"Third-party context type IntContext must be propagated to ForkJoin threads "
+ "(thousands digit should be 0) and thread priority (4) must be enforced "
+ "on ForkJoin threads (hundreds/tens/ones digits must be 4x5x9=180) "
+ "per configuration of the ManagedThreadFactoryDefinition and ContextServiceDefinition.");

@OndroMih
Copy link
Contributor

OndroMih commented Feb 5, 2024

As I commented on #212, I cannot support it. I believe behavior has never been clearly specified in the spec. GlassFish, which used to be the reference impl doesn’t comply with it.

I’d like to see opinions from other implementors and users of other implementations before I and the GlassFish team can support this.

@aubi
Copy link
Contributor Author

aubi commented Feb 6, 2024

@njr-11
Thanks a lot for your comments! I'll fix the issues.

@OndroMih

  1. I think that injection is the preferred way of using MES etc. So having some tests makes sense.
  2. Regarding the issue with storing context -- this test shows, what exactly I meant and I would like to clarify, how it should behave. But I'll fix the Nathan's issues first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants