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

workaround for node.js bug https://github.com/nodejs/node/issues/21398 #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timotab
Copy link

@timotab timotab commented Sep 6, 2019

There's a bug in the version of Node.js 10.x that Lambda uses that causes the http request 'end' event to not be fired. see nodejs/node#21398

This results in the "close" and therefore "total" timings to be the timeout value, instead of the actual URL load end value. Example graph (with calculated items to change from milliseconds to seconds) shows the results for www.google.com as I adjusted the timeout values: 6s, 10s, 30s, 60s.

google-timeouts

Workaround listed on the nodejs issues page applied to this code fixes the problem.

apparentorder added a commit to apparentorder/lambda-watchtower that referenced this pull request Jun 24, 2020
Per Node documentation, any response (http.IncomingMessage -> stream.Readable)
must be fully consumed. Otherwise, reading data will block, once the internal
buffer is full.

Effectively this caused http targets with a large enough response body to
always run until the configured timeout is reached.

This also causes the 'end' event not to fire.

References:

- https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_buffering:
  "Once the total size of the internal read buffer reaches the threshold
  specified by highWaterMark, the stream will temporarily stop reading data
  from the underlying resource until the data currently buffered can be
  consumed"

- https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_event_end:
  "The 'end' event will not be emitted unless the data is completely consumed."

Fixes wmnnd#11.
apparentorder added a commit to apparentorder/lambda-watchtower that referenced this pull request Jun 24, 2020
Per Node documentation, any response (http.IncomingMessage -> stream.Readable)
must be fully consumed. Otherwise, reading data will block, once the internal
buffer is full.

Effectively this caused http targets with a large enough response body to
always run until the configured timeout is reached.

This also causes the 'end' event not to fire.

References:

- https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_buffering:
  "Once the total size of the internal read buffer reaches the threshold
  specified by highWaterMark, the stream will temporarily stop reading data
  from the underlying resource until the data currently buffered can be
  consumed"

- https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_event_end:
  "The 'end' event will not be emitted unless the data is completely consumed."

Fixes wmnnd#11.
apparentorder added a commit to apparentorder/lambda-watchtower that referenced this pull request Jun 24, 2020
Per Node documentation, any response (http.IncomingMessage -> stream.Readable)
must be fully consumed. Otherwise, reading data will block, once the internal
buffer is full.

Effectively this caused http targets with a large enough response body to
always run until the configured timeout is reached.

This also causes the 'end' event not to fire.

References:

- https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_buffering:
  "Once the total size of the internal read buffer reaches the threshold
  specified by highWaterMark, the stream will temporarily stop reading data
  from the underlying resource until the data currently buffered can be
  consumed"

- https://nodejs.org/docs/latest-v10.x/api/stream.html#stream_event_end:
  "The 'end' event will not be emitted unless the data is completely consumed."

Fixes wmnnd#11.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant