fix: prevent chain effects in pushes #11
Conversation
Fixes google/node-fastify-auto-push#23. When recording the request patterns, don't use a path as a push key when it is also included in a push list for another key. It may cause a chain effect, causing unnecessary pushes. And it seems to crash the server sometimes. And also attach an error handler to the push stream.
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
==========================================
- Coverage 92.74% 90.37% -2.38%
==========================================
Files 3 3
Lines 124 135 +11
Branches 24 24
==========================================
+ Hits 115 122 +7
- Misses 9 13 +4
Continue to review full report at Codecov.
|
|
/cc @mcollina |
| }); | ||
| }; | ||
| const onError = (err: Error) => { | ||
| console.error('push stream error for:', asset, err); |
There was a problem hiding this comment.
I want to leave some logs when an error occurs during push. Do you think it should use some logging framework?
There was a problem hiding this comment.
this library should either emit an event 'pushError' or accept an option that will be called when this (and maybe other events) happen. fastify has its own logging thing included, see req.log
There was a problem hiding this comment.
'pushError' sounds good. I'll add that in a separate PR. Will also change the logging with fastify's logging thing. Thanks.
There was a problem hiding this comment.
Oh, sorry I was confused. This is a common package for auto-push, not specific to fastify. So it can't use fastify's logging.
I'll just add pushError.
There was a problem hiding this comment.
Yeah, this should emit pushError, and you should listen to the event in the fastify plugin and log it. You might want to emit the error on the parent stream maybe?
There was a problem hiding this comment.
pushStream is hidden from the client code so it doesn't make sense to emit pushError on pushStream. I'll emit it on the parent stream.
| reject(err); | ||
| }; | ||
| pushStream.once('error', onError); | ||
| pushStream.once('finish', onFinish); |
There was a problem hiding this comment.
I'm not 100% sure 'finish' is emitted when using respondWithFile cc @jasnell.
There was a problem hiding this comment.
I verified that onFinish is called. Otherwise this code wouldn't work because that is the only place where the promise is resolved.
Fixes google/node-fastify-auto-push#23.
When recording the request patterns, don't use a path as a push key when
it is also included in a push list for another key. It may cause a chain
effect, causing unnecessary pushes. And it seems to crash the server
sometimes.
And also attach an error handler to the push stream.