FLUID-6209: Model changes from multiple sources that affect each other can result in race conditions to set the model value.

Description

In the prefs framework we have a datastore where all of the model values are saved. If the settings are auto saved and the panel values are automatically updated from the store, you can have a case where changing the values quickly causes a race condition. This has been exhibited through the work on UIO+ on chrome in Windows 10. See https://issues.gpii.net/browse/GPII-2559

For examples of a stop gap solution, until the framework as a whole has better support for these types of cases, can be seen from work on pcpchannel.js

• https://github.com/amb26/universal/blob/13a8f45683f88c6cf5ae6ff68f3eee8dff5cbc05/gpii/node_modules/flowManager/src/PCPChannel.js#L75
• https://github.com/amb26/universal/blob/13a8f45683f88c6cf5ae6ff68f3eee8dff5cbc05/gpii/node_modules/flowManager/src/PCPChannel.js#L144

See Channel conversation: https://botbot.me/freenode/fluid-work/2017-10-04/?msg=91895959&page=1

Comments

• Justin Obara commented 2017-10-06T07:21:55.380-0400

  See @@Antranig Basman's suggestion from the fluid-work channel:

  https://botbot.me/freenode/fluid-work/2017-10-05/?msg=91953620&page=4

  Log:

  • 3:04 pm
    Justin_o - so my suggestion for now is that we adopt a protocol somewhat similar to the one in https://wiki.gpii.net/w/Nexus_API#Bind_Model
  • 3:04 pm
    Although we will want a few differences, some to be forward-looking to expected changes in the protocol, and some to adapt to our particular problem
  • 3:05 pm
    To start with, I suggest that you escape these message with an outer level of packaging, for a start holding a type field
  • 3:06 pm
    In this case, "type": "modelChanged"
  • 3:07 pm
    I suggest actually that you use this message sent by the "visible Nexus" as a prototype for your payload: https://github.com/amb26/fluid-authoring/blob/F...
  • 3:08 pm
    Only instead of "messageGeneration" you should put in a constantly increasing unique id "messageId"
  • 3:08 pm
    Then, in the other direction, you will send messages of type "modelChangedAck"
  • 3:08 pm
    Whose payload just consists of the id
  • 3:09 pm
    I think you can see where this is going 🙂
  • 3:12 pm
    One end of the bus will then maintain a hash of messaged, index by id, holding the unresolved promise corresponding to the model change
  • 3:12 pm
    The other end of the bus, once it has honoured the change, will send back a "modelChangedAck" back along the bus, and the corresponding promise will then get resolved and removed from the hash
  • 3:13 pm
    I expect this machinery won't in itself be sufficient to get rid of all of our races, but I think it is an essential part of it
  • 3:15 pm
    Well, perhaps this doesn't need to be as complex as this - it looks like a "port" probably guarantees that there can be at most one outstanding message at any time
  • 3:15 pm
    Although I don't so far see any written form of this guarantee : P
  • 3:17 pm
    These are the kinds of details which nerds never feel really necessary to write in their APIs
  • 3:17 pm
    Mainly because they have no idea how to express the types of them
  • 3:17 pm
    And if you can't express the type of something, it doesn't exist, right? : P
  • 3:19 pm
    I guess there's no reason to assume that at most one message can be outstanding

• Justin Obara commented 2017-10-06T10:48:24.230-0400

  Further discussion in the fluid-work channel:

  https://botbot.me/freenode/fluid-work/2017-10-06/?msg=91989326&page=1

• Justin Obara commented 2017-10-10T10:54:12.768-0400

  Further channel discussion:

  https://botbot.me/freenode/fluid-work/2017-10-10/?msg=92129943&page=1

• Justin Obara commented 2017-10-16T10:36:32.563-0400

  More discussion related to get/set https://botbot.me/freenode/fluid-work/2017-10-16/?msg=92362062&page=1

• Justin Obara commented 2017-10-19T12:05:21.973-0400

  Concern about debouncing using the onRead/onWrite chains.

  https://botbot.me/freenode/fluid-work/2017-10-19/?msg=92494854&page=1

• Justin Obara commented 2017-10-20T09:17:37.060-0400

  @@Antranig Basman based on our conversations there are three places I can think of to debounce the get/set requests for the datasource.

  1. The get/set invoker
     1. Could have hooks or some built-in mechanism that can handle debouncing but the flag is disabled by default or something like that
     2. Could require some extra invokers to wrap the get/set call, although this would seem to cause problems when moving between debounce and non-debounced versions
  2. The onRead.impl/onWrite.impl listener
     1. Would possibly require the implementer of the listener to handle debounce
     2. Could take the handler as an argument and use fluid.debounce as the handler, although this would add asymmetry with regards to the non-debounced version
     3. Perhaps we have a new type of listener that debounces calls to it's handlers
  3. Within the promise itself
     1. @@Antranig Basman mentioned possibly have cancellable promises, although it's not clear what the effect on the request chain would be

• Justin Obara commented 2017-10-27T07:33:39.066-0400

  Further discussion on debouncing ( https://botbot.me/freenode/fluid-work/2017-10-27/?msg=92806229&page=1 )

  Summary:

  Create a "bufferingModelComponent", it will know about two models, local and remote. If there is a discrepancy between them, it will attempt to synchronize.

  The remote model is updated via a model listener on the local model. The changes will be debounced and update the remote model after a write to the external source is confirmed (if the two models are the same, it will not actually attempt to perform a write). There will be a specific method for reading from the remote source (e.g. read(), pull(), fetch()). This will also be debounced and update the local model and remote model values at the same time.

  Note that read and write operations will block each other, only 1 request can be in flight at a time. This is to prevent issues where a read returns while a write operation is being performed.

• Justin Obara commented 2017-10-30T14:18:28.669-0400

  Further discussion about "bufferingModelComponent"

  https://botbot.me/freenode/fluid-work/2017-10-30/?msg=92913088&page=1

• Justin Obara commented 2017-10-31T08:59:10.589-0400

  Yes we still want only one in flight request at a time. Perhaps we'll need a queue for the request.

  https://botbot.me/freenode/fluid-work/2017-10-31/?msg=92945976&page=1

• Justin Obara commented 2017-10-31T14:47:09.164-0400

  Some discussion about a potential implementation.

  https://botbot.me/freenode/fluid-work/2017-10-31/?msg=92959036&page=2

• Justin Obara commented 2017-11-06T12:37:48.382-0500

  @@Antranig Basman and I had a discussion over Skype to further clarify and detail how to implement the model buffering component. This was captured in a gpii pad ( https://pad.gpii.net/p/deracing-model-updates-vi24nlk ).

• Justin Obara commented 2017-11-24T11:45:13.137-0500

  Comments from @@Antranig Basman in the fluid-work channel. https://botbot.me/freenode/fluid-work/2017-11-24/?msg=93879974&page=1

• Justin Obara commented 2018-02-27T08:57:21.114-0500

  I did some exploration into moving the afterFetch and afterWrite workflows into onFetch and onWrite respectively. The main issue preventing this is that the fireEventSequences used in the remote model, passes along the same payload to each listener ( https://github.com/jobara/infusion/blob/6c4381917a85641beb6195a50006976f7ebad733/src/framework/core/js/RemoteModel.js#L108-L126 ). This was done so that the listeners could be treated more like real listeners and not have to return back the payload at each step to the next listener in the chain. However, the onWrite and onFetch events don't have a payload. The payload was passed along in the afterWrite and afterFetch events.

• Cindy Li commented 2018-03-01T15:24:52.126-0500

  The pull request has been merged into the project repo master branch at 420dd5d9853f1121ad500e1acf02a185eaaa6b81