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

fix: race-condition that could leave stream in corked state #78

Merged
merged 1 commit into from Sep 2, 2019

Conversation

@watson
Copy link
Member

commented Sep 2, 2019

If running Node.js 10.2.0 or newer, a race condition would occur in the following scenarios that would leave the stream permanently corked:

  • Call flush while the stream is corked and bufferWindowTime !== -1
  • Call sendSpan, sendTransaction, sendError, or sendMetricSet while stream is corked, bufferWindowTime === -1 and _writableState.length >= bufferWindowSize.
fix: race-condition that could leave stream in corked state
If running Node.js 10.2.0 or newer, a race condition would occur in the
following scenarios that would leave the stream permanently corked:

- Call `flush` while the stream is corked and `bufferWindowSize !== -1`
- Call `sendSpan`, `sendTransaction`, `sendError`, or `sendMetricSet`
  while stream is corked, `bufferWindowTime === -1` and
  `_writableState.length >= bufferWindowSize`.

@watson watson self-assigned this Sep 2, 2019

@watson watson requested a review from Qard Sep 2, 2019

@watson

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

While reviewing this with @sqren we found that the stream would fix the issue by uncorking itself at some point when bufferWindowSize number of events had been written to the client. So this race-condition not as critical as I first thought. It just means that there will go a little more time until users see data in the APM UI

@sqren
sqren approved these changes Sep 2, 2019
Copy link
Member

left a comment

lgtm

@watson watson merged commit 913be08 into elastic:master Sep 2, 2019

6 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
CI-approved contributor
Details
CLA All commits in pull request signed
Details
Test Test passed
Details
apm-ci/pr-merge This commit looks good
Details
security/snyk - package.json (Qard) No manifest changes detected
security/snyk - package.json (watson) No manifest changes detected

@watson watson deleted the watson:cork branch Sep 2, 2019

@zube zube bot added [zube]: Done and removed [zube]: In Review labels Sep 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.