Block contextual operations from lazily enlisting CDI beans #8348

aguibert · 2019-07-24T20:13:06Z

Intermediate solution to the MP Context Propagation issue identified here:
eclipse/microprofile-context-propagation#167

LibbyBot · 2019-07-24T20:13:19Z

Code analysis and actions

DO NOT DELETE THIS COMMENT.

7 product code files were changed.
Please describe in a separate comment how you tested your changes.
2 FAT files were changed, added, or removed.
Check that the build did not break the affected FAT suite(s).
2 messages files were changed and need an L2 review.
@OpenLiberty/message-reviewer Please review.
dev/com.ibm.ws.concurrent.mp.1.0/resources/com/ibm/ws/concurrent/mp/resources/CWWKCMessages.nlsprops
dev/com.ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops
2 NLS files were changed and need an ID review.
@OpenLiberty/message-reviewer Please review.
dev/com.ibm.ws.concurrent.mp.1.0/resources/com/ibm/ws/concurrent/mp/resources/CWWKCMessages.nlsprops
dev/com.ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

njr-11 · 2019-07-24T20:56:58Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+# Contributors:
+#     IBM Corporation - initial API and implementation
+###############################################################################
+#CMVCPATHNAME com.ibm.ws.concurrent/resources/com/ibm/ws/concurrent/resources/CWWKCMessages.nlsprops


I'm not sure if this commented line matters, but com.ibm.ws.concurrent should be switched to com.ibm.ws.cdi.mp.context

it doesn't matter, it just can't be blank

njr-11 · 2019-07-24T21:14:29Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+
+# do not translate: CDI, CompletionStage
+CWWKC1158.cannot.lazy.enlist.beans=CWWKC1158E: CDI beans used in contextual operations, such as a CompletionStage, must be accessed using a CDI bean operation, such as a public \
+method, before context for the operation is captured. The CDI beans that were not accessed before context was captured are: {0}  


Would it be possible to state this directly in terms of what the user must do to fix the problem? For example,
"You must access CDI beans {0} prior to capturing context for the contextual action or task in order for the CDI beans to behave correctly when accessed from the action or task."
The way it is currently written makes it less clear that it is the user's/application's responsibility to take action to fix it.

I want to have {0} at the end of the message because it can get very long, because ContextualInstance toString() includes lots of information such as what qualifiers are on the bean. Also, a large list of beans could be provided.

I could change the last sentence from:
The CDI beans that were not accessed before context was captured are: {0}
to be:
The following CDI beans must be accessed before capturing context: {0}
to make it more clear what action the user needs to take

That's fine to put the list at the end. However, my comment about writing this more directly and including the word "you" still applies. Maybe we could combine the two?

"You must access CDI beans prior to capturing context for the contextual action or task in order for the CDI beans to behave correctly when accessed from the action or task. The following CDI beans must be accessed before capturing context: {0}."

It would be good to get the ID reviewer involved here to tell us what we can and cannot say in a message, and they may have some ideas for improving clarity as well.

njr-11 · 2019-07-24T21:30:18Z

dev/com.ibm.ws.concurrent.mp.1.0/src/com/ibm/ws/concurrent/mp/ContextualRunnable.java

@@ -68,6 +68,12 @@ public void run() {
            try {
                if (contextApplied != null)
                    threadContextDescriptor.taskStopping(contextApplied);
+            } catch (RuntimeException x) {
+                // prioritize surfacing an actual task failure over a failure clearing context


If we switch the prioritization in ContextualRunnable, we should do so in every other contextual action type as well (Function, BiFunction, Consumer, Supplier, ...). That said, I'm not convinced that we ought to switch the ordering. A failure to clear context is very severe, and typically would be more important than a task's own exception (which might even sometimes be expected). I would argue that also applies in the case for which these changes are being added.

njr-11 · 2019-07-24T21:31:04Z

...pplications/MPConcurrentCDIApp/src/concurrent/mp/fat/cdi/web/MPConcurrentCDITestServlet.java

+            }
+        }
+        assertTrue("CompletableFuture should have been marked done", cf.isDone());
+        assertTrue("CompletableFuture should have been marked as completex exceptionally", cf.isCompletedExceptionally());


minor typo: completex -> completed

skasund · 2019-07-25T15:35:29Z

L2 message review complete

ManasiGandhi

Suggested some minor changes.

ManasiGandhi · 2019-07-25T15:14:16Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+#
+# NLS_MESSAGEFORMAT_VAR
+#
+#   Strings in this file which contain replacement variables are processed by the MessageFormat 


Change
'Strings in this file which contain replacement variables are processed by the MessageFormat # class (single quote must be coded as 2 consecutive single quotes '').'
to
'Strings in this file, which contain replacement variables, are processed by the MessageFormat #class (single quotation mark must be coded as two consecutive single quotation marks '').'

this is a boilerplate header comment (not an NLS message) so I'm going to leave as-is. Any line that starts with # in a .nlsprops file is a comment and a user will never see it

ManasiGandhi · 2019-07-25T15:20:03Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+# NLS_MESSAGEFORMAT_VAR
+#
+#   Strings in this file which contain replacement variables are processed by the MessageFormat 
+#   class (single quote must be coded as 2 consecutive single quotes ''). Strings in this file 


Change
'Strings in this file, which do NOT contain replacement variables are NOT processed by the MessageFormat class'
to
'Strings in this file, which do NOT contain replacement variables are NOT processed by the MessageFormat class (single quotation mark must be coded as one single quote '). '
(single quote must be coded as one single quote '). '

this is a boilerplate header comment (not an NLS message) so I'm going to leave as-is. Any line that starts with # in a .nlsprops file is a comment and a user will never see it

ManasiGandhi · 2019-07-25T15:28:45Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+#   (single quote must be coded as one single quote '). 
+
+# This message bundle does not have its own message range, and instead reserves specific error messages from the 
+# com.ibm.ws.concurrent.mp.1.0 message bundle


Change
' This message bundle does not have its own message range, and instead reserves specific error messages from the com.ibm.ws.concurrent.mp.1.0 message bundle'.
to
'This message bundle does not have its own message range. Instead, it reserves specific error messages from the com.ibm.ws.concurrent.mp.1.0 message bundle '

this is a boilerplate header comment (not an NLS message) so I'm going to leave as-is. Any line that starts with # in a .nlsprops file is a comment and a user will never see it

ManasiGandhi · 2019-07-25T15:38:30Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+
+# do not translate: CDI, CompletionStage
+CWWKC1158.cannot.lazy.enlist.beans=CWWKC1158E: CDI beans used in contextual operations, such as a CompletionStage, must be accessed using a CDI bean operation, such as a public \
+method, before context for the operation is captured. The CDI beans that were not accessed before context was captured are: {0}  


Change
'CDI beans used in contextual operations, such as a CompletionStage, must be accessed using a CDI bean operation, such as a public method, before context for the operation is captured. The CDI beans that were not accessed before context was captured are: {0} '
to
' CDI beans used in contextual operations, such as a CompletionStage, must be accessed by using a CDI bean operation, such as a public method, before context for the operation is captured. The CDI beans that were not accessed before the context was captured are: {0} '

aguibert · 2019-07-25T16:09:04Z

#build

LibbyBot · 2019-07-25T16:09:13Z

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_KWl_YK7uEemoaqXqE6v4jQ

Target locations of links might be accessible only to IBM employees.

LibbyBot · 2019-07-25T17:32:29Z

Your Open Liberty build results are ready for viewing.

Unit test results are at http://public.dhe.ibm.com/ibmdl/export/pub/software/openliberty/runtime/personal/_KWl_YK7uEemoaqXqE6v4jQ/junit/index.html.
Gradle log and image zip are at http://public.dhe.ibm.com/ibmdl/export/pub/software/openliberty/runtime/personal/_KWl_YK7uEemoaqXqE6v4jQ.

njr-11 · 2019-07-26T12:52:57Z

#build

LibbyBot · 2019-07-26T12:53:10Z

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_8GicEK-bEemoaqXqE6v4jQ

Target locations of links might be accessible only to IBM employees.

LibbyBot · 2019-07-26T14:06:55Z

Your Open Liberty build results are ready for viewing.

Unit test results are at http://public.dhe.ibm.com/ibmdl/export/pub/software/openliberty/runtime/personal/_8GicEK-bEemoaqXqE6v4jQ/junit/index.html.
Gradle log and image zip are at http://public.dhe.ibm.com/ibmdl/export/pub/software/openliberty/runtime/personal/_8GicEK-bEemoaqXqE6v4jQ.

aguibert · 2019-07-28T19:40:22Z

#build

LibbyBot · 2019-07-28T19:40:32Z

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_LdmKELFnEemoaqXqE6v4jQ

Target locations of links might be accessible only to IBM employees.

LibbyBot · 2019-07-28T21:21:43Z

Your Open Liberty build results are ready for viewing.

Unit test results are at http://public.dhe.ibm.com/ibmdl/export/pub/software/openliberty/runtime/personal/_LdmKELFnEemoaqXqE6v4jQ/junit/index.html.
Gradle log and image zip are at http://public.dhe.ibm.com/ibmdl/export/pub/software/openliberty/runtime/personal/_LdmKELFnEemoaqXqE6v4jQ.

LibbyBot · 2019-07-29T01:13:46Z

The build aguibert-8348-20190728-1947
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_LdmKELFnEemoaqXqE6v4jQ
completed successfully!

ManasiGandhi

A few additional proposed changes.

ManasiGandhi · 2019-08-01T20:29:14Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+
+# do not translate: CDI, CompletionStage
+CWWKC1158.cannot.lazy.enlist.beans=CWWKC1158E:You must access CDI beans prior to capturing context for the contextual action or task in order for the CDI \
+beans to behave correctly when accessed from the action or task. The following CDI beans must be accessed before capturing context: {0}.


Change
'You must access CDI beans prior to capturing context for the contextual action or task in order for the CDI \ beans to behave correctly when accessed from the action or task. The following CDI beans must be accessed before capturing context: {0}.'
to
'The following CDI beans were not accessed before the context was captured: {0}. You must access the CDI beans before context is captured for the contextual action or task so that the CDI beans behave correctly. '

ManasiGandhi · 2019-08-01T20:39:52Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+CWWKC1158.cannot.lazy.enlist.beans=CWWKC1158E:You must access CDI beans prior to capturing context for the contextual action or task in order for the CDI \
+beans to behave correctly when accessed from the action or task. The following CDI beans must be accessed before capturing context: {0}.
+CWWKC1158.cannot.lazy.enlist.beans.explanation=When CDI context is captured, only CDI beans that have been accessed at that point in time have been created by \
+the CDI runtime. As a result, CDI beans that are first accessed in contextual operations cannot propagate their state correctly to subsequent contextual operations.


Change
'When CDI context is captured, only CDI beans that have been accessed at that point in time have been created by \ the CDI runtime. As a result, CDI beans that are first accessed in contextual operations cannot propagate their state correctly to subsequent contextual operations.'
to
'When the CDI context is captured, only CDI beans that are accessed at that point are created by the CDI runtime. As a result, CDI beans that are first accessed in contextual operations cannot propagate their state correctly to subsequent contextual operations.'

ManasiGandhi · 2019-08-01T20:44:46Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+CWWKC1158.cannot.lazy.enlist.beans.explanation=When CDI context is captured, only CDI beans that have been accessed at that point in time have been created by \
+the CDI runtime. As a result, CDI beans that are first accessed in contextual operations cannot propagate their state correctly to subsequent contextual operations.
+CWWKC1158.cannot.lazy.enlist.beans.useraction=The application must be changed to invoke some CDI bean operation, such as a public method, before context for \
+the operation is captured.


Change
'The application must be changed to invoke some CDI bean operation, such as a public method, before context for \ the operation is captured.'
to
'The application must be changed to start a CDI bean operation, such as a public method, before a context for the operation is captured.'

ManasiGandhi · 2019-08-01T20:45:19Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+#
+# NLS_MESSAGEFORMAT_VAR
+#
+#   Strings in this file which contain replacement variables are processed by the MessageFormat 


ManasiGandhi · 2019-08-01T20:45:25Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+# NLS_MESSAGEFORMAT_VAR
+#
+#   Strings in this file which contain replacement variables are processed by the MessageFormat 
+#   class (single quote must be coded as 2 consecutive single quotes ''). Strings in this file 


ManasiGandhi · 2019-08-01T20:45:36Z

....ibm.ws.cdi.mp.context/resources/com/ibm/ws/cdi/mp/context/resources/CDI_MP_CONTEXT.nlsprops

+#   (single quote must be coded as one single quote '). 
+
+# This message bundle does not have its own message range, and instead reserves specific error messages from the 
+# com.ibm.ws.concurrent.mp.1.0 message bundle


aguibert added the team:Zombie Apocalypse label Jul 24, 2019

aguibert requested a review from njr-11 July 24, 2019 20:13

aguibert self-assigned this Jul 24, 2019

LibbyBot added the CLA Signed label Jul 24, 2019

njr-11 reviewed Jul 24, 2019

View reviewed changes

skasund added the L2 Reviewed label Jul 25, 2019

ManasiGandhi reviewed Jul 25, 2019

View reviewed changes

njr-11 approved these changes Jul 25, 2019

View reviewed changes

aguibert modified the milestones: [Iteration 19.5] February 25 - March 8, [Iteration 19.15] July 15 - July 26 Jul 25, 2019

aguibert added 2 commits July 28, 2019 14:36

Block contextual operations from lazily enlisting CDI beans

486db60

Review comments

1404ad8

aguibert force-pushed the mpConcurrent-block-cdi-lazy-enlist branch from 1720ee8 to 1404ad8 Compare July 28, 2019 19:40

aguibert merged commit 6f172e2 into OpenLiberty:integration Jul 29, 2019

ManasiGandhi reviewed Aug 1, 2019

View reviewed changes

Block contextual operations from lazily enlisting CDI beans #8348

Block contextual operations from lazily enlisting CDI beans #8348

Conversation

aguibert commented Jul 24, 2019

LibbyBot commented Jul 24, 2019

Code analysis and actions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skasund commented Jul 25, 2019

ManasiGandhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aguibert commented Jul 25, 2019

LibbyBot commented Jul 25, 2019

LibbyBot commented Jul 25, 2019

njr-11 commented Jul 26, 2019

LibbyBot commented Jul 26, 2019

LibbyBot commented Jul 26, 2019

aguibert commented Jul 28, 2019

LibbyBot commented Jul 28, 2019

LibbyBot commented Jul 28, 2019

LibbyBot commented Jul 29, 2019

ManasiGandhi left a comment

Choose a reason for hiding this comment

ManasiGandhi Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ManasiGandhi Aug 1, 2019 •

edited

Loading