Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common.loggly can now call both the callback and success functions in the same run with loggly error #31

Open
blyork opened this issue Jan 8, 2018 · 3 comments

Comments

@blyork
Copy link

@blyork blyork commented Jan 8, 2018

A behavior change in 14324cf is causing common.loggly to potentially call both the callback and success functions in the event the Loggly servers come back with a error code but later returns a success.

Prior to this commit. Only one or the other function would ever be called.

This behavior is breaking both node-loggly-bulk's own Loggly.prototype.log function as well as winston-loggly-bulk's Loggly.prototype.log function which delegates to it since in both cases the callback passed in is only expected to be called once.

I am currently working on a pull request that modifies common.loggly to ensure that the callback and success functions combined only ever get called once.

I recommend changing common.loggy's signature so that it only accepts a single callback function. I do not believe this will cause issues to dependent third-party libraries since it is not exported as part of require('node-loggly-bulk'). I will create a separate issue for this recommendation for consideration.

@dguillamot
Copy link

@dguillamot dguillamot commented Jan 15, 2018

Hello, is this fix coming shortly? We are dependent on it to deploy Loggly again.

@blyork
Copy link
Author

@blyork blyork commented Jan 15, 2018

I put a pull request in back when I first found the issue: #33

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jun 22, 2018

Hi @blyork. This is done, can you please close this issue?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.