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

Simultaneous failures cause circuit to never move into the halfOpened state #385

Closed
mnutt opened this issue Nov 5, 2019 · 3 comments · Fixed by #386
Closed

Simultaneous failures cause circuit to never move into the halfOpened state #385

mnutt opened this issue Nov 5, 2019 · 3 comments · Fixed by #386

Comments

@mnutt
Copy link

mnutt commented Nov 5, 2019

Node.js Version:

10.17.0

Operating System:

MacOS 10.14.6

Steps to Produce Error:

I'm seeing an issue that started happening in 4.2.0 (#383) where the circuit never enters the halfOpened state.

If you have a circuit that automatically opens after 5 requests and you concurrently launch 6 simultaneous requests that each eventually error:

  • The first 4 requests will fail() but not call open()
  • The 5th request will call open() which will emit the open event and start the timer to later move into the halfOpened state
  • The 6th request will call open() again which will cancel the timer, but since it is already in the opened state, will not emit the open event and thus not restart the timer.
  • The circuit stays in the opened state and never enters the halfOpened state, so all further requests are rejected.

I think that the correct behavior might be for open() to cancel the current timer and immediately re-add it, so that it pushes the timer end date out regardless of whether or not the state change happens?

@tjenkinson
Copy link
Contributor

Oops sorry about that I didn’t realise open() was the same thing that was called internally. I thought for the public api it would be strange for an open call to later be undone

@tjenkinson
Copy link
Contributor

Another solution could be to check the state before calling open(), and only call it if it’s not already OPEN. This makes it still possible from the api to permanently open and close

lholmquist pushed a commit that referenced this issue Nov 8, 2019
fixes #385 by changing the behaviour of open() back to being a noop if the state is already open
@lholmquist
Copy link
Member

Just released this as 4.2.1

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