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

Callbacks called twice #25159

Closed
gluwer opened this issue Dec 21, 2018 · 4 comments
Closed

Callbacks called twice #25159

gluwer opened this issue Dec 21, 2018 · 4 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@gluwer
Copy link

gluwer commented Dec 21, 2018

  • Version: 8.14.0
  • Platform: 32-bit Windows
  • Subsystem: timers or http

We were using 8.11.1 on production servers for some time now. After an update to 8.14.0 some of the server we started to receive below exception (only on the updated ones):

Error: Callback was already called.
    at Timeout._onTimeout (D:\home\site\wwwroot\node_modules\async\lib\async.js:43:36)
    at ontimeout (timers.js:498:11)
    at tryOnTimeout (timers.js:323:5)
    at Timer.listOnTimeout (timers.js:290:5)

Async library checks if callback was fired twice which should not happen. The issue happens very rarely (once every 1-2 hours on one of many node processes handling 100 req/s), but sometimes it happens 3-4 times in a row in timespan of 100ms (the same node process). After reverting back to 8.11.1 issue is gone.

We cannot reproduce it locally to provide a sample test case. I assume it has to have something to do with recent security fixes and http module adding two setTimeouts or firing some event twice in case of http error/timeout (our node app is running in the cloud so this happens but rarely). I doubt that timer.js core module itself is the cause.

It may also be related to #23631, but there only 10.x is mentioned. We also do not think aby lib currently used by us refers to http2 module (only to http).

@mritunjayz
Copy link
Contributor

It seem like same as #23631 missed condition for 'false' value of 'self._writableState.errorEmitted' in If block.(line

if (self._writableState.errorEmitted)
)

'self._writableState.errorEmitted' have to be 'True' after each callback.

@gluwer
Copy link
Author

gluwer commented Dec 28, 2018

Sorry for the delay. Will this fix be backported to 8.x?

@Trott
Copy link
Member

Trott commented Dec 28, 2018

Sorry for the delay. Will this fix be backported to 8.x?

@gluwer On the one hand, b94ce57 does not cherry-pick cleanly to the 8.x-staging branch. And it landed in October, so you'd think that if it was going to land in 8.x, it would have landed by now. So that suggests "no". However...

On the other hand, I don't see any dont-land-on tags in #23631. And 8.x is in Active Maintenance for at least a few more days, if not a few more weeks. So a report like this one along the lines of "this is a bug that is affecting me" might warrant a backport?

Ultimately, it's probably up to some combination of: @nodejs/releasers @nodejs/backporters @cjihrig

@targos targos added the v8.x label Jul 21, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Node.js v8.x has reached the end-of-life and won't receive any fixes anymore. I am closing this since this issue only applies to Node.js v8.x. Other release lines received a fix.

No matter if you run into this issue or not, please update to a newer Node.js version in case you still use v8.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants