This repository was archived by the owner on Feb 13, 2024. It is now read-only.
Ensure callback is called when invoking errorHanlder#347
Merged
pooyaj merged 1 commit intosegmentio:masterfrom Aug 2, 2022
Merged
Ensure callback is called when invoking errorHanlder#347pooyaj merged 1 commit intosegmentio:masterfrom
pooyaj merged 1 commit intosegmentio:masterfrom
Conversation
A property, errorHanlder, was recently added which will be called
instead of throwing an error in the .flush() method. This is important
because errors in .flush() could, previously, only be handled via
process.on('uncaughtException', err => { ... }).
However, this property is currently unusable as, when the flush method
invokes this property, it fails to call the callbacks of the events
being flushed.
This commit makes sure the callbacks are called.
Author
|
I'm not sure if it's important that I open an issue before a PR. If it is, I'm happy to retroactively open an issue. |
timhaley94
commented
Jul 28, 2022
| t.true(errorHandler.calledOnce) | ||
| }) | ||
|
|
||
| test('flush - evoke callback when errorHandler option is specified', async t => { |
Author
There was a problem hiding this comment.
Did this TDD style. If you comment out the done(err) in index.js, this test will fail.
pooyaj
approved these changes
Aug 2, 2022
Contributor
|
thanks for the fix 🙌 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I work at Trello @ Atlassian.
We use this library, however, it started causing problems for us when
throw errwas added to the.flush()method. This has been documented here. (#320) For us it was resulting in process crashes when we receive an error message (like a 429) when emitting an event. We had to pin the version of this library to a version before this behavior was added.I saw, that the
errorHandlerproperty was added (#342) and was attempting to update our code to use it, however, when writing a test case, I saw that my event callback was never invoked whenerrorHandlerwas present and calledThis is because, in the
.flush(), everywhere else the method can end, we calldone(err), however we do not when looking for and invoking theerrorHandler. To reiterate the importance, it's a core bit of functionality of this library that the callbacks for events are invoked when the messages are flushed, so it's needs to occur in this instance as well.