privacybydesign / irma-frontend-packages

Collection of frontend related packages, that together form a Javascript "client" to the IRMA server.
7 stars 4 forks source link

Re-design of state machine API to prevent that developers introduce race conditions #40

Closed ivard closed 3 years ago

ivard commented 3 years ago

We want to get rid of the possibility for developers of plugins (like we are ourselves, but also devs that make custom plugins) to introduce race conditions. This change is based on the discussions in PR #35 .

Problem

The transition function used to be blocking. This causes problems when you call transition from within stateChange of one of the plugins. This is a quite logical thing to do in some situations, for example when you have to deal with an error. Lets assume we have plugin a and b begin registered at the state machine and we are in a state change for transition x and plugin a wants to throw an error:

Stack trace from the perspective of plugin a:

Here you have the weird behaviour that processing of the old state change continues after the new state has already been processed. This means that plugin a is processing a state change of a state in the past.

Stack trace from the perspective of plugin b: From the perspective of plugin b it is even more weird:

Here the state change is fully executed in the wrong order. Developers might not be aware that state changes can come in in any order, so this makes it very hard to reason about it. Practically, you have to support the state change from x to y for every possible state x and y. This makes no sense.

Proposed change

I made the transition function async, without using promise chaining. This means that transitions are added to the JavaScript event loop, making sure state changes are executed in the right order. I explicitly don't use promise chaining. When allowing promise chaining, the state change might get scattered over multiple events in the event loop, making it again possible that state changes get mixed up.

Making it async, makes it harder to reason about a state change when you want to do it conditionally. For example, if you check that transition x is possible now and you initiate the transition, you are not sure that transition x is still valid when it is actually performed. A transition of another plugin might have intervened.

Therefore I deprecate the getState, isEndState and isValidTransition methods and replace them by a selectTransition method that makes it possible to do the validity check on the exact moment the transition is scheduled. This prevents any race condition to happen.

ivard commented 3 years ago

I furthermore removed the functionality that a transition automatically falls back to fail in cause you initiate one that is invalid. With selectTransition this should not happen anymore, so better keep it simple and always return an error if you do something invalid.

Because of this change the fail transition is superfluous in the Uninitialized state now.