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

Fix managedthreadfactory component context #213

Merged

Conversation

aubi
Copy link
Contributor

@aubi aubi commented Apr 26, 2022

Obvious copy&paste error:
txTaskResult -> lookupTaskResult.completeExceptionally()

Ad java:comp usage: is there any particular reason to use it in this test? I has a specific behavior in web app (equal to module), but EJB-bound in EAR app. If we want to use it, it must be kept EJB-bound, e.g. lookup it in the managedThreadFactoryDefinitionBean. Personally, I would prefer to use java:app as it makes me better sense to define thread factory for whole app.

@aubi aubi force-pushed the fix-managedthreadfactory-component-context branch from c6ad053 to 595be6e Compare April 26, 2022 15:25
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

Obvious copy&paste error:
txTaskResult -> lookupTaskResult.completeExceptionally()

Yes, definitely appears to be a copy/paste error.

Ad java:comp usage: is there any particular reason to use it in this test? I has a specific behavior in web app (equal to module), but EJB-bound in EAR app. If we want to use it, it must be kept EJB-bound, e.g. lookup it in the managedThreadFactoryDefinitionBean.

This test does seem awkward and complex and real applications wouldn't write their code this way. I can guess the reason for using java:comp here is to be another test of ManagedThreadFactory capturing context upon lookup. Because the ManagedThreadFactory lookup occurs within the ManagedThreadFactoryDefinitionBean EJB, threads created by the ManagedThreadFactory must run with that EJB's context and be capable of performing java:comp lookups as though they are running from the EJB, so InitialContext.doLookup("java:comp/concurrent/EJBThreadFactoryB") within the task is actually valid and should not need to change.

Normally in this situation I'd ask to keep only the first change that fixes the copy/paste error and not the second (or if we had more time, replace the second with a better test). However, needing to make any change at all at this point puts us at risk of not making the deadline for spec approval for EE 10, and we certainly don't have the time for a back and forth discussion about whether to make the second change and getting the pull updated. So, because the second change is at least still valid, I'll approve and merge it so that we can the new TCK built and certified on and hopefully still be able to publish the results before the deadline passes to make EE 10.

@njr-11 njr-11 merged commit 1acd002 into jakartaee:master Apr 26, 2022
@aubi
Copy link
Contributor Author

aubi commented Apr 26, 2022

@njr-11 You are right with splitting the changes, sorry.

ad context: The thread is executed from ManagedThreadFactoryDefinitionOnEJBServlet, e.g. the component doesn't match ManagedThreadFactoryDefinitionBean. Also, it is in EAR.

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.

None yet

2 participants