Fix _writev to only call its callback when all chunks are done #50
Conversation
Fixs github issue winstonjs#49 This allows stream events such as 'finish' to work correctly on the transport stream instead of possibly triggering before the individual log calls have been finished.
|
Interesting approach, thanks for taking the time to submit a PR. Let's hope a suitable fix makes it to Winston in the next point-release. It is a shame to rely on a hack. |
|
I would just like to feedback that this PR works for me. It works as intended. |
|
@montdidier Just out of curiosity, what did you test? Did you have some code before that used to break and you used this patch and saw that it didn't afterward? |
|
@laganojunior I have an AWS lambda that logs to papertrail. I kept hitting write after end errors even though I was only exiting the lambda on I am not sure if that is exactly what happened but, short answer, yes, I had existing broken code that this fixed. I was about to abandon winston because it just seems extremely fragile/brittle. |
index.js
Outdated
| @@ -151,18 +167,16 @@ TransportStream.prototype._writev = function _writev(chunks, callback) { | |||
|
|
|||
| if (errState || !transformed) { | |||
| // eslint-disable-next-line callback-return | |||
| chunks[i].callback(); | |||
| wrapCallback(i); | |||
There was a problem hiding this comment.
This should be wrapCallback(i)(), because this code change replaces the chunks[i].callback() function call, not the function reference chunks[i].callback (like the other two instances in this PR).
There was a problem hiding this comment.
Good catch! This probably didn't come up in the tests because there were no errors. Thanks, I'll fix that up.
|
if this patch is correct, why is not it merged? The flushing trouble is very important. |
|
Any update? |
|
Any idea why CI is failing? |
Fixs github issue #49
This allows stream events such as 'finish' to work correctly on the
transport stream instead of possibly triggering before the individual
log calls have been finished.