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
FLUID-6209: Fix race conditions for Prefs framework #867
Conversation
The dataSource was taken from kettle's datasource-core with some modifications to use pseudo events inplace of getImpl/setImpl and using grade linkage instead of getWriteable grade.
Updated to work with all forms of listeners including ones with changePaths.
This allows for a workflow that automatically triggers a fetch before/after a write, during a write request.
If this isn't fired, the write request doesn't unblock.
@amb26 could you please review this PR to address the race conditions in the prefs framework. |
transaction.fireChangeRequest({path: "local", type: "DELETE"}); // clear old local model | ||
transaction.change("local", that.model.remote); // reset local model to the base for applying changes. | ||
transaction.fireChangeRequest({path: "remote", type: "DELETE"}); // clear old remote model | ||
transaction.change("remote", fetchedModel); // update remote model to fetched changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, yes, yes
transaction.change("local", that.model.remote); // reset local model to the base for applying changes. | ||
transaction.fireChangeRequest({path: "remote", type: "DELETE"}); // clear old remote model | ||
transaction.change("remote", fetchedModel); // update remote model to fetched changes. | ||
fluid.fireChanges(transaction, changes); // apply changes from remote and local onto base model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about you, but to me something seems interestingly and fundamentally right about this (even though there may well still be bugs in detail)
src/framework/core/js/RemoteModel.js
Outdated
transaction.commit(); // submit transaction | ||
}; | ||
|
||
fluid.remoteModelComponent.makeSequneceStrategy = function (payload) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo here
src/framework/core/js/RemoteModel.js
Outdated
|
||
fluid.remoteModelComponent.makeSequneceStrategy = function (payload) { | ||
return { | ||
invokeNext: function (that) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add detailed comment here contrasting this implementation with the standard one (referencing its location in the code) and explaining why this improvement was necessary. Ideally we should just say that the standard impl has a bug/limitation and write it up as a JIRA and fix it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are referring to the use of apply. I have actually fixed this in FluidPromise as well, in this PR. https://github.com/jobara/infusion/blob/ca255c90147329a2a277c3c47efc267ac6f913f3/src/framework/core/js/FluidPromises.js#L209
The new makeSequenceStrategy was actually needed because this is a different type of sequence. Rather than passing along the result from one listener to the next, it actually passes along the original payload to each. This is because the synthetic events are treated more like typical events, but we need to know when they are all finished before resolving/rejecting the promise.
I've added a comment to try to explain the difference.
src/framework/core/js/RemoteModel.js
Outdated
return sequence.promise; | ||
}; | ||
|
||
fluid.remoteModelComponent.fetch = function (that) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Substantial comments needed on both fetch and write - ideally we should write up the indented pseudocode we worked on and polish it a bit. Perhaps some kind of tree diagram/sequence diagram?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 I've added comments to the fetch and write functions. I've created the following diagrams that can be added to the documentation that should be written for the fluid.remoteModelComponent
.
@@ -30,7 +30,7 @@ var fluid_3_0_0 = fluid_3_0_0 || {}; | |||
onScrollDelay: 100, // in ms, used to set the delay for debouncing the scroll event relay | |||
model: { | |||
// panelMaxIndex: null, // determined by the number of panels calculated after the onPrefsEditorMarkupReady event fired | |||
panelIndex: 0 | |||
// panelIndex: 0 // the index of the panel to open on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to have picked up some stray material from another pull - can we rebase this against current master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I needed to comment that line out in order for the panelIndex to be properly pulled in from the store. Otherwise this was overriding things back to 0, because of the model relay I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there's no connection between the work in this pull and the ArrowScrolling work, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I see that there is. We should try to put the default value back in, and prevent it from propagating out with an excludeSource: "init" directive on the relay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 I think I'm remembering more of the problem. If there is a default value, it will be applied instead of the value from the cookie (or other datastore). If exclude init, then you can't pass in a panelIndex through the component settings to have it default to a different panel.
Although now that I'm testing this out by uncommenting the default value for panelIndex, I'm getting a strange error at https://github.com/jobara/infusion/blob/ca255c90147329a2a277c3c47efc267ac6f913f3/src/framework/preferences/js/PrefsEditor.js#L352 It seems that the changeMap has the value "ADD". This results in the following error "DataBinding.js:496 Uncaught TypeError: Cannot read property '0' of undefined"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually an ok value for a ChangeMap and the prefsEditor code has a bug. A ChangeMap isn't a flat map, it's an object isomorphic to the model with values "ADD" and "DELETE" at the locations where changes have occurred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 I've tried to exclude init for the panelIndex relay, but it seems to cause the issue I mentioned above, where it ignores values for the panelIndex in the components defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is a pretty interesting issue - do you think you could write up the details of how this situation works as a JIRA, with a title something like "model relay system cannot distinguish between different sources of initial values"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 I've filed a JIRA https://issues.fluidproject.org/browse/FLUID-6249 Let me know if you'd like any more information provided.
@amb26 I've started writing up the docs for the changes in this PR. So far I have written documentation for the two new files: DataSource and remoteModelComponent. NOTE: If you click on the diagrams you'll see them rendered correctly. |
src/framework/core/js/RemoteModel.js
Outdated
(function ($, fluid) { | ||
"use strict"; | ||
|
||
fluid.defaults("fluid.remoteModelComponent", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do with some substantial comments here explaining what is a "remote model" given clearly this component itself is local : P Perhaps with reference to some diagrams/workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started writing documentation for the remoteModelComponent. I'll add a link, but it won't be live yet as the documentation needs to be merged in still.
src/framework/core/js/RemoteModel.js
Outdated
fluid.defaults("fluid.remoteModelComponent", { | ||
gradeNames: ["fluid.modelComponent"], | ||
events: { | ||
afterFetch: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having separate events for errors is ok, but I think separate onWrite/afterWrite and onFetch/afterFetch errors is dubious, because of the standard problem of associating together the particular firings of related events - that is, there is no clear way for a user to identify the particular firing of an afterFetch event that corresponds to a particular onFetch. For this reason I think we should combine these events and just have a more elaborate namespace structure within the fewer events that allows users to schedule things at a time of interest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 could you expand on what you mean by "have a more elaborate namespace structure within the fewer events that allows users to schedule things at a time of interest"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke with @amb26 in the IRC channel about this ( https://botbot.me/freenode/fluid-work/2018-02-26/?msg=97241487&page=1 ). The suggestion is to try to move the items from the afterWrite and afterFetch to be part of the onWrite and onFetch sequence. However, the current implementation is working, so will timebox the exploration of adjusting the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -41,7 +41,9 @@ var fluid_3_0_0 = fluid_3_0_0 || {}; | |||
} | |||
}, | |||
listeners: { | |||
"onReady.boilOnPrefsEditorReady": "{fullPreview}.events.onPrefsEditorReady" | |||
"onReady.boil": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly this looks like stray material from another pull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that it will properly override the "onReady.boil" listener from the prefsEditorLoader base grade. https://github.com/jobara/infusion/blob/6c4381917a85641beb6195a50006976f7ebad733/src/framework/preferences/js/PrefsEditor.js#L47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see
panelIndex: "{separatedPanel}.model.panelIndex", | ||
panelMaxIndex: "{separatedPanel}.model.panelMaxIndex", | ||
local: { | ||
panelIndex: "{that}.model.panelIndex" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the distinction between the local panelIndex and the top-level one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 this is the setup for what is tracked for saving between the local and remote models, as part of the fluid.remoteModelComponent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - it would be useful to add a comment on line 150 reminding the user that this structuring arises through the use of fluid.remoteModelComponent - and that what it is for is arranging for this to be persisted and synchronised there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amb26 that makes sense. I've added a comment about it.
Added link to the upcoming documentation.
Added back the default value for panelIndex.
@amb26 I've reverted the change to the panelIndex model value, it is commented out again and documented under FLUID-6249. I've also updated with the master and fixed merge conflicts. This is ready for more review. |
@amb26 ready for more review. |
https://issues.fluidproject.org/browse/FLUID-6209
This PR Adds:
It also updates the prefs framework's stores to use the fluid.dataSource component and the prefsEditor to be a fluid.remoteModelComponent