feat: initialize the state of a breaker on creation

[lholmquist]

This PR is a continuation of #568. That last PR only allowed a user to prime the circuit breaker with the stats of the circuit. This part adds the ability to prime the breaker with a starting state. To make it easier for users to see there state, and ultimately save it some where, the exportState function has been added.

To create a Circuit with states, there is now a state parameter in the options. Creating a new circuit might look like this:

const state = {
....
}


const breaker = new CircuitBreaker({state: state});



// to export the state

const breakerState = breaker.exportState();

I updated the README with some samples of how this would work

The only part i'm not thrilled with is but is probably ok, is that during initilization, if you prime a circuit to be open, then the code emits an "open" event so that the proper timers runs, but it also increments the status count of open, even though the protected function never ran. It probably isn't even an issue since it only will increment the status by 1.

[lance]

Overall, I like it. I do have some thoughts on field duplication and other concerns that were not driving motivators with this change, but instead historical cruft that was just highlighted as the cruft that it is with this PR. I also wonder if it makes sense to completely separate state and status, so have made a suggestion about combining these two into a single state option that contains a status.

[lance]

I wonder if ultimately, HALF_OPEN is all that's needed. The PENDING_CLOSE state is only read once in all of circuit.js. Have you tried removing the PENDING_CLOSE state and just have getPendingClose() return this.halfOpen? If that passes the snuff test, I suggest we remove PENDING_CLOSE from the exported state altogether.

[lholmquist]

It seems that they are almost the same thing. I did that quick test and it looks like we can probably remove pendingClose and just go with halfOpen

[lance]

These changes are touching a lot of areas that I think have been bothering me for a while. One is this weirdness. Why do we have both this[OPEN] and this[CLOSED]? It just opens the door for update anomolies and race conditions. I wonder if this PR is a good opportunity to revisit some of the decisions made early on in development.

If you do decide to keep this double-state, I think one should always be opposite of the other, which isn't the case here. It's worth considering if they are both needed, though.

[lholmquist]

Why do we have both this[OPEN] and this[CLOSED]?

Well, you wrote it :) i would have to look a little deeper to see if both are needed. maybe that would be in a different PR though

We probably don't need to pass both in. Since if it isn't closed, then it is Open. I would opt for only using the closed parameter

[lance]

I think this should stay as the default, and in the if (options.state) block, it can be updated if needed.

[lholmquist]

sounds good

[lance]

This should all be in the above if (options.state) {} block, shouldn't it? And could probably be simplified to something like this.

Suggested change

	// Prepopulate the State of the Breaker
	if (this[CLOSED]) {
	// If the state is closed, we don't do anything?
	this[STATE] = CLOSED;
	this.close();
	} else if (this[OPEN]) {
	// If the state being passed in is OPEN
	// THen we need to start some timers
	this.open();
	} else if (this[HALF_OPEN]) {
	// Not sure if anythign needs to be done here
	this[STATE] = HALF_OPEN;
	} else if (this[SHUTDOWN]) {
	// IF the state of the circuit being passed in is SHUTDOWN
	// Then i don't think we need to do anything either?
	this[STATE] = SHUTDOWN;
	this.shutdown();
	}
	if (options.state.shutdown) {
	this.shutdown();
	} else if (options.state.open) {
	this.open();
	}
	if (options.state.halfOpen) {
	this[STATE] = HALF_OPEN;
	}

[lholmquist]

I'm pretty sure there was a reason for doing this way. I think it was becuase there are event listeners and functions that need to get created first, before this code gets executed. but i'll have to go back and check that

[lance]

Barring my comments above about field/behavior duplication, I like this. It got me thinking that a toJSON() method might be useful. E.g.

Suggested change

	exportState () {
	return {
	enabled: this.enabled,
	closed: this.closed,
	open: this.opened,
	halfOpen: this.halfOpen,
	warmUp: this.warmUp,
	pendingClose: this.pendingClose,
	shutdown: this.isShutdown
	};
	}
	exportState () {
	return this.toJSON();
	}
	// ensures that when a circuit is serialized as JSON, it looks like this
	toJSON() {
	return {
	name: this.name,
	group: this.group,
	enabled: this.enabled,
	opened: this.opened,
	halfOpen: this.halfOpen,
	warmUp: this.warmUp,
	shutdown: this.isShutdown,
	status: this.status // obviously this would affect the ctor params
	};
	}

This also ensures that a circuit's state and status on import/export are in sync.

[lholmquist]

@lance i've made some updates. I added the toJSON method and removed the exportState function since they do the same thing. I also added the status.stats to the toJSON function so it now exports something like this:

{
  state: { ... },
 status: { ... }
}

Which doesn't require any changes to the constructor.

I decided not to mess around with the pendingClose/HalfOpen stuff since that can be done in a future update since it is not user facing.

[lance]

I decided not to mess around with the pendingClose/HalfOpen stuff since that can be done in a future update since it is not user facing.

Probably a good idea.