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 EventEmitter leak in dev-runtime #752

Merged
merged 5 commits into from Apr 4, 2019

Conversation

Projects
None yet
4 participants
@cdlewis
Copy link
Contributor

commented Apr 3, 2019

Lifecycle registered two event listeners using once. Only one would
unregister itself, leading to us leaking emitters. This could
potentially bring down the Fusion server as calls to lifecycle.wait()
would hang.

Fix EventEmitter leak in dev-runtime
Lifecycle registered two event listeners using `once`. Only one would
unregister itself, leading to us leaking emitters. This could
potentially bring down the Fusion server as calls to `lifecycle.wait()`
would hang.
Show resolved Hide resolved build/dev-runtime.js Outdated

cdlewis added some commits Apr 4, 2019

@KevinGrandon KevinGrandon added the ci label Apr 4, 2019

@UberOpenSourceBot UberOpenSourceBot removed the ci label Apr 4, 2019

Bump timeout for server-startup-control
Test is passing locally.

@KevinGrandon KevinGrandon added the ci label Apr 4, 2019

@UberOpenSourceBot UberOpenSourceBot removed the ci label Apr 4, 2019

@micburks micburks added the ci label Apr 4, 2019

@UberOpenSourceBot UberOpenSourceBot removed the ci label Apr 4, 2019

@micburks
Copy link
Contributor

left a comment

Looks great!

@micburks

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

!merge

@fusion-bot fusion-bot bot merged commit f6e0bb6 into fusionjs:master Apr 4, 2019

19 checks passed

buildkite/fusion-cli Build #1751 passed (5 minutes, 53 seconds)
Details
buildkite/fusion-cli/docker-package Passed (1 minute, 37 seconds)
Details
buildkite/fusion-cli/docker-package-node8 Passed (1 minute, 20 seconds)
Details
buildkite/fusion-cli/eslint Passed (33 seconds)
Details
buildkite/fusion-cli/eslint-node8 Passed (32 seconds)
Details
buildkite/fusion-cli/flowtype Passed (33 seconds)
Details
buildkite/fusion-cli/flowtype-node8 Passed (33 seconds)
Details
buildkite/fusion-cli/node-white-check-mark Passed (3 minutes, 51 seconds)
Details
buildkite/fusion-cli/node-white-check-mark-node8 Passed (3 minutes, 48 seconds)
Details
buildkite/fusion-cli/pipeline Passed (7 seconds)
Details
ci-gate Pull Request accepted for CI
license/cla Contributor License Agreement is signed.
Details
probot/label-docs-pr Docs label has been set (or unset)
probot/label-release-pr Release label has been set (or unset)
probot/migrations Migration guide provided
probot/pr-label At least one required semver-related label exists
probot/pr-title PR title is valid
probot/release-verification Verification not required for this pull request.
probot/todos All TODOs have open issues
@fusion-bot

This comment has been minimized.

Copy link

commented Apr 4, 2019

@@ -17,7 +17,7 @@ test('`fusion dev` proxy gracefully recovers from cached SSR errors', async () =
async function waitForCompileToStart() {
// Once the Fusion.js application renders SSR error pages in the app
// we should leverage module.hot.addStatusHandler.
await new Promise(resolve => setTimeout(resolve, 3000));
await new Promise(resolve => setTimeout(resolve, 10000));

This comment has been minimized.

Copy link
@KevinGrandon

KevinGrandon Apr 5, 2019

Contributor

Is this still necessary? Seems this is not a timeout, and adjusting this has a slight performance impact on tests. (Hopefully in the future we change this so we don't need hard-coded number at all)

This comment has been minimized.

Copy link
@cdlewis

cdlewis Apr 5, 2019

Author Contributor

Yeah this was probably unnecessary. I put up a PR to undo #757

KevinGrandon added a commit that referenced this pull request Apr 5, 2019

See if we can revert setTimeout change
Introduced in #752, just want to see if this is necessary since that PR made additional changes afterwards. Not much impact here, just adds some extra time in the test chunk.

@cdlewis cdlewis deleted the cdlewis:clewis-event-emitter branch Apr 5, 2019

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