Ensure ResultMessage is processed

[jiangpengcheng]

Description

As processCompeltion and processResult is executed asynchronously, it's better to ensure ResultMessage will be processed in any case

Related issue and scope

• I opened an issue to propose and discuss this change (the processAcknowledgement may process CompletionMessage first for blocking incovation even the message is sent ahead of ResultMessage #4134 )

My changes affect the following components

• API
• Controller
• Message Bus (e.g., Kafka)
• Loadbalancer
• Invoker
• Intrinsic actions (e.g., sequences, conductors)
• Data stores (e.g., CouchDB)
• Tests
• Deployment
• CLI
• General tooling
• Documentation

Types of changes

• Bug fix (generally a non-breaking change which closes an issue).
• Enhancement or new feature (adds new functionality).
• Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

• I signed an Apache CLA.
• I reviewed the style guides and followed the recommendations (Travis CI will check :).
• I added tests to cover my changes.
• My changes require further changes to the documentation.
• I updated the documentation where necessary.

[codecov-io]

Codecov Report

Merging #4135 into master will decrease coverage by 4.89%.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##           master    #4135     +/-   ##
=========================================
- Coverage   86.17%   81.28%   -4.9%     
=========================================
  Files         151      151             
  Lines        7271     7272      +1     
  Branches      469      468      -1     
=========================================
- Hits         6266     5911    -355     
- Misses       1005     1361    +356

Impacted Files	Coverage Δ
...e/loadBalancer/ShardingContainerPoolBalancer.scala	85.09% <85.71%> (ø)	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	0% <0%> (-100%)	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	0% <0%> (-95.54%)	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	0% <0%> (-92.6%)	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	0% <0%> (-83.34%)	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	0% <0%> (-62.5%)	⬇️
...in/scala/org/apache/openwhisk/common/Counter.scala	40% <0%> (-20%)	⬇️
...penwhisk/core/database/cosmosdb/CosmosDBUtil.scala	92% <0%> (-4%)	⬇️
...rg/apache/openwhisk/common/ForcibleSemaphore.scala	92.3% <0%> (-3.85%)	⬇️
...isk/core/controller/actions/PrimitiveActions.scala	86.71% <0%> (-0.7%)	⬇️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bd4971...f4c6540. Read the comment docs.

[cbickel]

@jiangpengcheng Thanks a lot for tackling this issue again.
I also looked at the problem again. What do you think about the following approach of fixing this:
The promise is only used in case the activation was blocking. In case of non-blocking activations, this promise is never used.
What about storing the promise only in your new map for blocking requests?

[jiangpengcheng]

@cbickel
do you mean create an extra Promise[Either[ActivationId, WhiskActivation]] , saved it into the new map for blocking activations, and return this promise.future in the publish method,
and for non-blocking activations, just keep the original way?

or

don't create promise for non-blocking activations, just return an Future.successful(Left(aid)) in the publish method?

[cbickel]

LGTM
Thanks a lot for doing this PR and simplifying it.

[cbickel]

Ah wait. One more thing came into my mind. Do we still need to store the promise in the ActivationEntry with this change? I think it would be enough to store it in blockingPromises, right?

[jiangpengcheng]

yes, we don't need that promise

[ningyougang]

@cbickel ,can be merged?

[rabbah]

while not likely, it's plausible the active ack is received before the promise is added to this map (which is why setup activation adds the promise before it's posted to any invoker).