Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

Don't start a timer #172

Merged
merged 3 commits into from Aug 18, 2021
Merged

Don't start a timer #172

merged 3 commits into from Aug 18, 2021

Conversation

chris-pardy
Copy link

Resolves #121

When the queue has hit the maximum length don't start a timer. If a timer exists flush will cancel it. Timers in node are part of the "event loop" as long as work is in the event loop the node process won't exit, this has impacts to things like AWS Lambda where you're effectively spinning up and shutting down runtimes very quickly.

When the queue has hit the maximum length don't start a timer. If a
timer exists flush will cancel it.
@codecov-io
Copy link

Codecov Report

Merging #172 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #172   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         108    109    +1     
=====================================
+ Hits          108    109    +1
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6b53bb...ba2343d. Read the comment docs.

Copy link
Contributor

@f2prateek f2prateek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM! But I would love to have somebody else also confirm that using this in a lambda to verify this change does indeed fix the issues they were having.

@amirrustam
Copy link

This would also be helpful for bull queue workers. Generally it would nice if the batching feature can be disabled via a config option.

@uccmen
Copy link

uccmen commented Dec 3, 2018

we are seeing socket hang up errors when setting flushAt: 1. If I understand correctly - this PR will help us resolve this issue as the timer is contributing to the node event loop.

[ERROR] { Error: socket hang up
at createHangUpError (_http_client.js:331:15)
at TLSSocket.socketOnEnd (_http_client.js:423:23)
at emitNone (events.js:111:20)
at TLSSocket.emit (events.js:208:7)
at endReadableNT (_stream_readable.js:1064:12)
at _combinedTickCallback (internal/process/next_tick.js:138:11)
at process._tickDomainCallback (internal/process/next_tick.js:218:9)
code: 'ECONNRESET',

@uccmen
Copy link

uccmen commented Jul 1, 2020

was this merged?

@pbassut
Copy link
Contributor

pbassut commented Apr 19, 2021

As @f2prateek pointed out. We needed someone to run this PR on a lambda and check if it doesn't hang.
I assume @chris-pardy closed it because it was never merged? Or because there was something wrong with it?

@chris-pardy
Copy link
Author

I think I'd closed it since it hadn't gotten merged for several years. Happy to have someone else take it over the finish line though.

@nd4p90x nd4p90x reopened this Jul 24, 2021

await delay(10)
t.true(client.flush.calledTwice)
})
Copy link
Contributor

@pbassut pbassut Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not clear enough.
If I understood this correctly: even though it waited 10 seconds it didn't flush a third time because it had already flushed when it reached flushAt.
Maybe the title could be enqueue - prevent flushing through time interval when already flushed by flushAt

test.js Outdated Show resolved Hide resolved
@nd4p90x nd4p90x added this to In progress in analytics-node Jul 26, 2021
@nd4p90x nd4p90x moved this from In progress to Needs Review in analytics-node Jul 28, 2021
@nd4p90x nd4p90x requested a review from pooyaj July 28, 2021 19:01
Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>
@pooyaj pooyaj merged commit 3fbcb3c into segmentio:master Aug 18, 2021
analytics-node automation moved this from Needs Review to Done Aug 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Timer is scheduled to flush even if queue is empty
9 participants