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

feat: optional timeout (#200) #202

Merged
merged 1 commit into from
Jun 6, 2018

Conversation

duartemendes
Copy link
Contributor

No description provided.

@ghost ghost added the in progress label May 28, 2018
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

There is currently a default timeout value, so this will not do anything unless the user provides options.timeout = null (or undefined or 0). This is the way I would prefer it, but I just want to clarify that is the case.

Can you please add a test for this?

@lance lance self-assigned this May 28, 2018
@duartemendes
Copy link
Contributor Author

duartemendes commented May 29, 2018

I'm aware of the default value. On my project, I'm setting the timeout as false, so it's more clear and the default behaviour is still the same, so we aren't doing any breaking changes.

Not sure how I could test this, any tips @lance?

@lance
Copy link
Member

lance commented May 30, 2018

@duartemendes it's non-optimal because of the fact that it will take > 1 second, but here is a stake in the ground.

test('timeout is not called', t => {
  t.plan(1);
  // set up circuit breaker with timeout disabled
  const cb = circuitBreaker(timedFunction(3000), { timeout: false } );
  cb.on('timeout', t.fail);

  // 1000ms is the default timeout value
  // this test passes if we exceed this timeout value
  setTimeout(t.ok, 1500);
  cb.fire();
});

EDIT Fix example so it makes sense

If you could also add a note to the docs indicating this behavior, I would appreciate it. Thanks!

@lance
Copy link
Member

lance commented May 31, 2018

@duartemendes I've made these changes locally but can't push to your branch. Here is a diff: https://gist.github.com/lance/6a630d182cdad6ec8456bf3ee42afbcd

@duartemendes
Copy link
Contributor Author

duartemendes commented Jun 4, 2018

@lance sorry for the delay, went for a little vacation.

I understand your test, however isn't the default timeout 10000ms? :)

I tried the test with timedFunction as 12000 and timeout as 15000 but tests started failing:

 Allows timeout to be optional

✔ function called

✖ plan != count
----------------
  operator: fail
  expected: 1
  actual:   2
  at: circuit.healthCheck._ (/projects/opossum/test/health-check-test.js:11:7)
  stack: |-


✖ .end() called twice
----------------------
  operator: fail
  at: circuit.healthCheck._ (/projects/opossum/test/health-check-test.js:12:7)
  stack: |-

✔ health-check-failed emitted

✖ plan != count
----------------
  operator: fail
  expected: 1
  actual:   2
  at: CircuitBreaker.circuit.on.e (/projects/opossum/test/health-check-test.js:21:7)
  stack: |-


✖ .end() called twice
----------------------
  operator: fail
  at: CircuitBreaker.circuit.on.e (/projects/opossum/test/health-check-test.js:22:7)
  stack: |-

✔ function called

✖ .end() called twice
----------------------
  operator: fail
  at: circuit.healthCheck._ (/projects/opossum/test/health-check-test.js:12:7)
  stack: |-

✔ health-check-failed emitted

✖ .end() called twice
----------------------
  operator: fail
  at: CircuitBreaker.circuit.on.e (/projects/opossum/test/health-check-test.js:22:7)
  stack: |-

✔ should be truthy

Updated the docs though.

@lance
Copy link
Member

lance commented Jun 6, 2018

@duartemendes OMG - an extra zero makes a lot of difference. Thanks for your help with this!

@lance lance merged commit 7611d6f into nodeshift:master Jun 6, 2018
@ghost ghost removed the in progress label Jun 6, 2018
@lance lance mentioned this pull request Jun 6, 2018
@duartemendes
Copy link
Contributor Author

No problem @lance! Please update package on npm :)

@duartemendes duartemendes deleted the 200-optional-timeout branch June 6, 2018 15:06
@lance
Copy link
Member

lance commented Jun 6, 2018

@duartemendes 1.7.0 is now available on npmjs.com and here https://github.com/bucharest-gold/opossum/releases/tag/v1.7.0

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

Successfully merging this pull request may close these issues.

2 participants