Fix promise leaks

[scottinet]

Description

The functions exposed by the notifier core generate many promises that are not awaited, and whose rejections might not be handled. And this is bad practice.

But more than a bad practice, this is also a real issue because, well... hm... I'll let Node.js do the explaining:

(node:32263) UnhandledPromiseRejectionWarning: Unhandled promise rejection. 
  This error originated either by throwing inside of an async function without a 
  catch block, or by rejecting a promise which was not handled with .catch(). 
  (rejection id: 1)
(node:32263) [DEP0018] DeprecationWarning: Unhandled promise rejections are 
  deprecated. In the future, promise rejections that are not handled will 
  terminate the Node.js process with a non-zero exit code.

That kind of warning doesn't look nice in Kuzzle's logs, not mentioning the potential issue this might cause if Node.js starts crashing on unhandled promise rejections.

How to test

Check the unit tests logs: no more warnings 🎉

[codecov]

Codecov Report

Merging #1311 into 1-dev will increase coverage by <.01%.
The diff coverage is 89.62%.

@@            Coverage Diff            @@
##            1-dev   #1311      +/-   ##
=========================================
+ Coverage   93.79%   93.8%   +<.01%     
=========================================
  Files          98      98              
  Lines        6967    6991      +24     
=========================================
+ Hits         6535    6558      +23     
- Misses        432     433       +1

Impacted Files	Coverage Δ
lib/api/core/auth/tokenManager.js	91.07% <100%> (+0.5%)	⬆️
lib/api/controllers/realtimeController.js	96.77% <100%> (+0.1%)	⬆️
lib/api/core/hotelClerk.js	92.97% <100%> (+1.35%)	⬆️
lib/api/controllers/documentController.js	99.16% <100%> (+0.04%)	⬆️
lib/api/core/notifier.js	72.93% <70.83%> (-0.67%)	⬇️

---

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 d23e5df...2b337aa. Read the comment docs.

[Aschen]

I didn't understand why it's needed to add the index and the collection to the cache 🤔
If they did not already exists, Kuzzle will have reject the request and if they exists, they are already in the cache (populated at startup or added when creating the collection)

[scottinet]

I didn't pay attention to that line, since it was already there. But now that you mention it, I can't find why this would be useful.

That line was added by a 3-years old PR and my best guess is that, at that time, it was useful to put in cache the indexes/collections created directly in ES while Kuzzle was running to prevent false negatives.

This is not useful anymore since the index cache now features a hotreload mechanism.

So I'll remove it. Good catch 👍

[benoitvidis]

I agree we need to handle rejections issued by the notifier but I am a little concerned with 2 points:

1. Waiting for the Promise chain to resolve will have an impact on Kuzzle response time. I understand Koncorde is fast and we let the protocols handle the final dispatch without waiting for it but still, we have to wait for quite a bunch of operations, including some possible pipes from plugins.
2. I will check but it is not very clear to me if we cover all the cases where the notifier could raise an error while being sure it won't eventually be sent to the end-user.

I did not think yet on how we can tackle these possible issues or whether they are acceptable or not. What do you think?

[scottinet]

@benoitvidis > the first issue is not that straightforward: what kind of performances are we measuring?
This might impact the response times of API requests where the real-time engine is involved, because now Kuzzle responds only when notifications are actually computed and submitted to the network layer.
But in the other hand, that means that we free up a request slot only after all promises are settled. And this might solve a backpressure problem, where promises generated by the real-time engine can pile up in the event loop because we didn't wait before processing another request from the buffer.
So, my take on this is that:

1. it might have a slight negative impact on API response times where real-time is involved
2. it might have a slight positive impact on notifications delay
3. it might prevent Kuzzle's event loop from being saturated

I'm using the conditional here, because in fact, I couldn't see any impact when I ran our benchmarks. So these considerations are theoretical, as far as I can see at least, and that's probably because the real-time engine is a lot faster than, say, checking user rights.

About the second issue: I'm not sure, but I think that we probably will have new errors appearing once the new Kuzzle version is released, because the warnings we got came from unhandled promise rejections.
So, after this PR, those errors should still be unhandled I guess, but at least they should now be properly logged or returned to requesting users, so we should now be able to fix those.

[scottinet]

Re-reading my answer, I agree that there are unknows with this PR. Do you know how we could be a bit more sure of the outcomes?
On my side, I'll try to see if I can think of something...

[benoitvidis]

OK, I had not thought about the back-pressure issue and the fact the benchmarks do not show any difference is a good news :)