Dac0d3r
Dac0d3r

Reputation: 1854

MobX action schedule / execution order isn't preserved due to race condition

Here's a running example of what I've got so far:

https://codesandbox.io/s/github/BruceL33t/mobx-action-synchronous-execution-order/tree/master/

store.js:

import { observable, action } from "mobx";
import Sensor from "../models/Sensor";

export default class RootStore {
  @observable sensors = new Map();

  constructor() {
    let self = this;
    const sensorIds = [
      "sensor1",
      "sensor2",
      "sensor3",
      "sensor4",
      "sensor5",
      "sensor6",
      "sensor7",
      "sensor8",
      "sensor9",
      "sensor10"
    ];

    for (let sensor of sensorIds) {
      self.sensors.set(sensor, new Sensor(5));
    }

    // setInterval simulates some incoming data (originally from SignalR, and roughly each second)
    setInterval(function() {
      let out = {};
      const x = +new Date(); // unix timestamp
      for (let sensor of sensorIds) {
        const y = Math.floor(Math.random() * 10000) + 1;
        const m = { x: x, y: y };
        out[sensor] = m;
      }

      self.addMeasurement(out); // the problem starts here.
    }, 1000);
  }

  // the problem!
  @action
  addMeasurement(sensorMeasurementMap) {
    let self = this;
    // this timeout is to try and simulate a race condition
    // since each measurement is incoming each second,
    // here some of them will take as long as 6 seconds to add,
    // due to the timeout.
    // the point is that they should always be added,
    // in the order they were called in.
    // so if the first measurement takes 20 seconds to be added,
    // the next measurements that were received on 2, 3, 4, 5..., 19th second etc,
    // should all "wait" for the prev measurement, so they're added
    // in the right order (order can be checked by timestamp, x)
    setTimeout(() => {
      const keys = self.sensors.keys();

      if (keys.length === 0) {
        // never really gonna happen, since we already set them above
      } else {
        for (const key in sensorMeasurementMap) {
          if (self.sensors.keys().indexOf(key) > -1) {
            self.sensors.get(key).add(sensorMeasurementMap[key]);
          } else {
            // also not gonna happen in this example
          }
        }
      }
    }, Math.floor(Math.random() * 20 + 1) * 1000);
  }
}

Sensor.js:

import Queue from './Queue';
import {observable, action} from 'mobx';

export default class Sensor {
  @observable queue;

  constructor(n) {
    this.n = n;
    this.queue = new Queue(this.n);
  }
  @action add(measurement) {
    this.queue.add(measurement);
  }
}

Queue.js:

import {observable, action} from 'mobx';

export default class Queue {
  @observable data;
  constructor(maxSize) {
    this.maxSize = maxSize;
    this.size = 0;
    this.data = [];
  }

  @action add(measurement) {
    let removedItem = undefined;
    if(this.size >= this.maxSize) {
      let temp = this.data[0];
      removedItem = temp && temp.y ? temp.y+'' : undefined;
      this.data.shift();
    }

    this.data.push(measurement);

    if (removedItem === undefined && this.size < this.maxSize) {
      this.size++;
    }

    return removedItem;

  }
}

There's a few comments in the code but you absolutely need to see the output https://codesandbox.io/s/github/BruceL33t/mobx-action-synchronous-execution-order/tree/master/ to understand it.

Let me also try to explain it here, what this is all about.

This is basically a overly simplified version of a part of a real application, where setInterval is just used instead to simulate a SignalR event handler to indicate incoming data each second. The incoming data is what we create inside the setInterval func above the addMeasurement action.

So given some incoming data is received each second, we want to add this to the observable map sensors on the store. Since this data is used for drawing charts in the real application, we need to make sure it is indeed added in the order which the actions are invoked in - no matter how long the action takes to complete.

In the real application I saw some inconsistency in the order of how the data were pushed to the MobX state, so I isolated it and extracted the relevant parts into this example and tried to exaggerate it a bit by using the setTimeout func inside the addMeasurement action.

Since each data is fetched each second, but some measurements could take up to 20 seconds to fetch (not realisticly, but to clearly show race condition problem), as the code is right now, it often happens that we end up with something like:

[
    {"x":1519637083193,"y":4411},
    {"x":1519637080192,"y":7562},
    {"x":1519637084193,"y":1269},
    {"x":1519637085192,"y":8916},
    {"x":1519637081192,"y":7365}
]

Which really should never happen, since 1519637083193 is greater/later than 1519637080192.

This is a real problem when drawing charts from this data and ordering it afterwards is way too expensive, so I'm looking for a way to improve this code so we can trust each addMeasurement is only fired once the previous action has completely finished. Or at least a way to update the MobX state in the right order

Hope it makes sense.

Upvotes: 1

Views: 867

Answers (1)

mweststrate
mweststrate

Reputation: 4978

should all "wait" for the prev measurement, so they're added in the right order (order can be checked by timestamp, x).

Could you elaborate on that? How could one ever know that no timestamp larger than the current one will be received in the future, and hence wait indefinitely? Isn't what you are looking for just a sorted insertion to the array of measurements (instead of waiting)?

If sorted insertion doesn't solve the problem, I would probably do the following (untested):

lastAddition = Promise.resolve() // start with already finishied addition

addMeasurement(sensorMeasurementMap) {
      this.lastAddition = this.lastAddition.then(() => {
          return new Promise((resolve, reject) => {
            setTimeout(action(() => {
                const keys = self.sensors.keys();

                if (keys.length === 0) {
                    // never really gonna happen, since we already set them above
                } else {
                    for (const key in sensorMeasurementMap) {
                    if (self.sensors.keys().indexOf(key) > -1) {
                        self.sensors.get(key).add(sensorMeasurementMap[key]);
                    } else {
                        // also not gonna happen in this example
                    }
                    }
                }
                resolve()
            }), Math.floor(Math.random() * 20 + 1) * 1000);
        })
      })
  }
}

N.B.: Note that I moved action inside, as you need it at the place where you are actually modifying the state, not where the scheduling happens

Upvotes: 1

Related Questions