Bug 1153002

In ProcessJPATest.java, this test has 2 blocks for completing a task. If you change the place of transactionManager.begin() in BOTH blocks (= move out getRuntimeEngine() from the transaction), the issue would go away.

====
 79             // complete task
 80             {
 81 //                transactionManager.begin();
 82                 RuntimeEngine runtime = manager.getRuntimeEngine(EmptyContext.get());
 83 
 84                 TaskService taskService = runtime.getTaskService();
 85 
 86                 transactionManager.begin();
 87 
 88                 List<TaskSummary> list = taskService.getTasksAssignedAsPotentialOwner("john", "en-UK");
 89                 TaskSummary taskSummary = list.get(0);
 90                 System.out.println("john starts/completes a task : taskId = " + taskSummary.getId());
 91                 taskService.start(taskSummary.getId(), "john");
 92                 taskService.complete(taskSummary.getId(), "john", null);
 93 
 94                 transactionManager.commit();
 95     
 96                 manager.disposeRuntimeEngine(runtime);
 97             }
 98     
 99             // complete task
100             {
101 //                transactionManager.begin();
102                 
103                 RuntimeEngine runtime = manager.getRuntimeEngine(EmptyContext.get());
104                 
105                 TaskService taskService = runtime.getTaskService();
106                 
107                 transactionManager.begin();
108             
109                 List<TaskSummary> list = taskService.getTasksAssignedAsPotentialOwner("john", "en-UK");
110                 TaskSummary taskSummary = list.get(0);
111                 System.out.println("john starts/completes a task : taskId = " + taskSummary.getId());
112                 taskService.start(taskSummary.getId(), "john");
113                 taskService.complete(taskSummary.getId(), "john", null);
114 
115                 transactionManager.commit();
116 
117                 manager.disposeRuntimeEngine(runtime);
118             }
====

the problem in that case is wrong usage of PerProcessInstance strategy as you should not use EmptyContext when retrieving RuntimeEngine when PerProcessInstance strategy is used. You need to always use 
ProcessInstanceIdContext.get(processInstanceId)

the only exception to this rule is when you start the process as there is no context mapping available so you should use
ProcessInstanceIdContext.get()

With that change I can run the reproducer without an issue.

Maciej,

I am afraid that another customer can run into this issue again. I think that it could be a good idea to throw an exception, which would tell the user that it is not possible to use PerProcessInstance strategy with empty context. Unfortunately throwing a NPE is not informative enough and looks like a bug.

What do you think?

EmptyContext is still allowed to be used when first time getting runtime engine when startProcess is to be called. But not when process instance exists so I guess adding additional validation in there is a good idea. Will look into it.

Hi Maciej,

Please imagine that those operations ("start process"/"complete task"/"complete task") are triggerted by different http requests. The user doesn't know the process instance id which is associated to the task when they try to complete a task.

I thought that using a taskService of a different process instance's (or empty) RuntimeEngine is okay and then ExternalTaskEventListener (https://github.com/droolsjbpm/jbpm/blob/6.0.x/jbpm-human-task/jbpm-human-task-workitems/src/main/java/org/jbpm/services/task/wih/ExternalTaskEventListener.java#L50) finds the right PerProcessInstance RuntimeEngine to proceed the process instance. If it's not true, we should not get a taskService from PerProcessInstance RuntimeEngine for the purpose?

My thoughts are:

A) A user needs to know taskId + processInstanceId of the task to start/complete so we can getRuntimeEngine with processInstanceId. (processInstanceId can be acquired from TaskSummary so it's not a difficult change to the user application)

B) Create a TaskService from HumanTaskServiceFactory instead of getting from a RuntimeEngine.

Thanks!
Toshiya

Toshiya,

whole point of RUntimeManager and then RuntimeEngine is that you should not think about ksession and task service (and others that are managed by the RuntimeEngine) as independent components. They should always be bootstrapped as one component. 

Even though ExternalTaskEventListener can load it but it won't load it properly as it will rely on transaction scoped (ThreadLocal) instance that will be preloaded on getRuntimeEngine.

In 6.1 this is slightly enhanced as ksession is not loaded on getRuntimeEngine invocation but on demand meaning when it's needed so it cannot be reproduced on master (6.1) either.

Nevertheless we should promote usage of RuntimeEngine as an engine without asking users to think about task service, ksession independently as they are "managed".

Furthermore when you look at services implementations you'll notice that it does apply the logic that you described, always use scoped runtime engine whenever us needed and task service is used from RUntimeEngine whenever possible - exceptions are quick tasks as they are not bound to any process instance.

Does that make sense to you?

(In reply to Jiri Svitak from comment #3)
> Maciej,
> 
> I am afraid that another customer can run into this issue again. I think
> that it could be a good idea to throw an exception, which would tell the
> user that it is not possible to use PerProcessInstance strategy with empty
> context. Unfortunately throwing a NPE is not informative enough and looks
> like a bug.
> 
> What do you think?

Jiri,

as described in the previous comment a number of improvements has been applied in 6.1, and one of the biggest is to load ksession on demand to prevent it from being unnecessary fetched from db. Current impl allows to use both ProcessInstanceContext and EmptyContext for initial RuntimeEngine creation - when no process instance is available. Changing that now might cause issues for code (outside of our control) to fail. I suggest to add a logger warning whenever such situation is encountered and inform it's not recommended and will be removed with next major version. Would that work for you guys?

Thank you, Maciej.

Please double-check my understanding:

* When you use PerProcessInstanceRuntimeManager:

1) Use a taskService acquired from RuntimeEngine only when you know the related process instance id

2) Use an independent taskService when you don't know the related process instance id
 -> It can be injected by HumanTaskServiceProducer if you use jbpm-kie-services
 -> If you don't use jbpm-kie-services, you can create a HumanTaskService from HumanTaskServiceFactory

3) For read only operations (e.g. getTasksAssignedAsPotentialOwner), you may use any of 1 and 2. (or 2 is recommended?)

I think this is an important 'best practice' for a jbpm embedded application. Thank you for your help!

Toshiya,

you're right, that's a fair statement

Thanks, Maciej!

> I suggest to add a logger warning whenever such situation is encountered and inform it's not recommended and will be removed with next major version. Would that work for you guys?

Thanks, +1 for WARN message.

(In reply to Maciej Swiderski from comment #7)
> Jiri,
> 
> as described in the previous comment a number of improvements has been
> applied in 6.1, and one of the biggest is to load ksession on demand to
> prevent it from being unnecessary fetched from db. Current impl allows to
> use both ProcessInstanceContext and EmptyContext for initial RuntimeEngine
> creation - when no process instance is available. Changing that now might
> cause issues for code (outside of our control) to fail. I suggest to add a
> logger warning whenever such situation is encountered and inform it's not
> recommended and will be removed with next major version. Would that work for
> you guys?

Just to be sure - I assume that current impl is the BPMS 6.1 version and added warning is logged when user starts a process with EmptyContext used.
I hope I got it right. Thus I am OK with this approach.

My general point was just that the product should always avoid NullPointerException and throw a more informative exception instead. Of course I understand that this NPE will not be fixed in current 6.0 product serie.

Toshiya, can we close this bug?

Yes, I'm closing this BZ as NOT_A_BUG.