PDA

View Full Version : [CLOSED] Duplicate records when calling sync() on a autoSync store



ruslan.talpa
2 Nov 2011, 1:18 AM
REQUIRED INFORMATION


Ext version tested:

Ext 4.0.7

Browser versions tested against:

____
FF7.0.1

DOCTYPE tested against:

____

Description:

If you define a store with autoSync and also call .sync() after adding a record, that record will be added two times. The same will happen if you have autosync disabled but call sync() two times in a row

Steps to reproduce the problem:


The result that was expected:

The store should know it is already in the sync process and handle the situation when the second sync is called

The result that occurs instead:

each operation on a record (add update delete) is duplicated, it is not that critical on update/delete but create is a problem

Test Case:



<<insert working code to reproduce the report >>




HELPFUL INFORMATION


Screenshot or Video:

attached

See this URL for live test case: http://


Debugging already done:

none

Possible fix:


The quick fix would be to have a "syncInProgress" flag for a store when a sync starts set that to true and if the second sync is called, postpone it using a timer for 10-50 ms.

The better solution would be to have a "isBeingSynced" flag for each model so that functions like getNewRecords() know not to return the second time around models that have already been sent to the backend (but the operation did not finish yet). This situation is especially useful and visible when working with a direct store that has enableBuffer enabled and different processes work with the same store.

Additional CSS used:

only default ext-all.css
custom css (include details)

Operating System:

________
WinXP Pro

ruslan.talpa
2 Nov 2011, 2:00 AM
This is the fix, hope this (or a better version) gets moved into Ext





Ext.override(Ext.data.Model, {
isBeingSynced: false
});


Ext.override(Ext.data.AbstractStore, {
filterNew: function(item) {
// only want phantom records that are valid and are not being synced at this moment
return item.phantom === true && item.isValid() && !item.isBeingSynced;
},

filterUpdated: function(item) {
// only want dirty records, not phantoms that are valid
return item.dirty === true && item.phantom !== true && item.isValid() && !item.isBeingSynced;
},

sync: function() {
var me = this,
i,
options = {},
toCreate = me.getNewRecords(),
toUpdate = me.getUpdatedRecords(),
toDestroy = me.getRemovedRecords(),
needsSync = false;


if (toCreate.length > 0) {
options.create = toCreate;
needsSync = true;
}


if (toUpdate.length > 0) {
options.update = toUpdate;
needsSync = true;
}


if (toDestroy.length > 0) {
options.destroy = toDestroy;
needsSync = true;
}


if (needsSync && me.fireEvent('beforesync', options) !== false) {
if (toCreate.length > 0) {
for(i = 0; i < options.create.length; i++){
options.create[i].isBeingSynced = true;
}
}
if (toUpdate.length > 0) {
for(i = 0; i < options.update.length; i++){
options.update[i].isBeingSynced = true;
}
}
me.proxy.batch(options, me.getBatchListeners());
}
},
});

mszukajt
2 Nov 2011, 5:23 AM
I agree, this is the issue.

Ruslan's proposition push us in the right direction but it could be much better to use existing STORE.loading internal flag to do not allow any "loading" actions (delete, create, update, load) and show a "load" mask in a view/grid during any of those operations, additionally.

isBeingSynced flag still can be very usefull on a model level, as is proposed, when somebody tries to fire action on a specific record, out of the store.

mitchellsimoens
2 Nov 2011, 6:40 AM
I will be pushing this to our bug tracker but if you have autoSync to true, why would you use sync() then?

mszukajt
2 Nov 2011, 7:06 AM
I will be pushing this to our bug tracker but if you have autoSync to true, why would you use sync() then?

Well, imagine this situation (is real in my case):
You have button on toolbar for example called "save" and set store to autosync (with another toggle button for example as option for user or somehow... never mind, you know what I mean).
User changes record which is send to remote side. But there is no any mask or other information that record has been send.
User clicks "save" button before sync operation finishes (this all is called asynchronous!). And we are in trouble...
Even you do not have set autsync to true how to assure user will not click save button and fire sync event twice?


Yes, I know, I can set some info in toolbar, some notificatin window, keep sync/load state in grid/view, etc. but still, this should be handled internally in the store not in separate logic somewhere in the view or controller, don't you think?

ruslan.talpa
3 Nov 2011, 2:43 AM
@mszukajt
I don't think that blocking "write" requests to the server is a good idea, this will make the communication with the server slow, when there is the simple solution above, plus i do not think that a store should be so "linked" to a view that we have to show a spinner each time it's loading something. We have the network speed and technology (ext direct) to make the app seem almost instantaneous.

Ext store should handle code like this without the developers having to write custom code or "be careful" about the code:


store.add(someModel);
store.sync();
store.add(anotherModel);
store.sync()


Now you would say, why not call sync() only at the end.... but what if you have that type of code in different functions/listeners and they all make some modification to the store then call sync ... it becomes a little more complicated to managing when to call sync() in your code.

In addition to this i also have a fix (or maybe call it a feature) to Proxy and Batch classes so that they can send the data to the server in asynchronous mode (at this time it is synchronous, the batch will wait for one operation to finish before sending another)

This is quite useful when you have a direct enabled api with enableBuffer, you could send a lot of "write" operations in just one request.

mszukajt
3 Nov 2011, 4:28 AM
@Ruslan


First of all, your post from second paragraph to end is to Mitchell I guess? Because it looks we're thinking about the same...


I am not sure if it has to be blocked but it has to be synchronized somehow when using THE SAME store.

Your solution does not cover situation when somebody sends request to "save" data and loads something in the meantime (new page for example). What will you show to user when using the same store?

Simple example from TreeStore:


onUpdateRecords: function(records, operation, success){
if (success) {
var me = this,
i = 0,
length = records.length,
data = me.data,
original,
parentNode,
record;


for (; i < length; ++i) {
record = records[i];
original = me.tree.getNodeById(record.getId());
parentNode = original.parentNode;
if (parentNode) {
// prevent being added to the removed cache
original.isReplace = true;
parentNode.replaceChild(record, original);
original.isReplace = false;
}
}
}
},


Try to sync some changes in records and reload whole tree in the meantime. You will get this error:



Uncaught TypeError: Cannot read property 'parentNode' of undefined
Ext.define.onUpdateRecords ext-all-dev.js:80426
Ext.define.onProxyWrite ext-all-dev.js:66426
Ext.define.onBatchOperationComplete ext-all-dev.js:66465
fire ext-all-dev.js:17461
Ext.define.continueFireEvent ext-all-dev.js:21702
Ext.define.fireEvent ext-all-dev.js:21675
onProxyReturn ext-all-dev.js:76389
Ext.define.processResponse ext-all-dev.js:40203
(anonymous function) ext-all-dev.js:40682
Ext.apply.callback ext-all-dev.js:9218
Ext.define.onComplete ext-all-dev.js:28462
Ext.define.onStateChange ext-all-dev.js:28400
(anonymous function) ext-all-dev.js:2421



Nowadays network's speed is a good thing but only in intranet applications. You can not make this assumption otherwise.
And even the best network is nothing when server ( WEB or DB ) response is bad - and it happens. Should I say to my client they have to spend next $100k for new/better server because DB response is now 3 seconds and allows user to do some "bad things"?
Not every application is dedicated for instantaneous access nor every application uses data from "transactional" DB model (faster access by indexes, etc.), but still we have to manage those "bad things" somehow.
Show users there is still working something, at least.

ruslan.talpa
3 Nov 2011, 5:13 AM
I was referring to locking the store for each operation (if a sync is running then lock the second one).
If the discussion is about locking a "load" when a sync is in progress then you are right, it should be locked (much like operations on a DB table)

As to the last paragraph, you are right, situations like that occur, i was only against showing a "loader" on a view each time the store is communicating with the backend (that would be annoying, but it would be helpful when you are performing a load)

mszukajt
3 Nov 2011, 6:36 AM
Ah, ok then. That was kind of misunderstanding and we are really talking about the same :).

I agree the Update/Create/Destroy (U/C/D) operations on records in store should be able to fire independently (asynchronous). Ext.data.Batch processes it synchronously now.

So, postulates to store:
1. There should be some mechanism to not allow to process record which is already processed somehow.
2. There should be lock between write (U/C/D) and load operations.
3. There should be possible to run U/C/D operations during sync() call in asynchronous mode.

I am afraid this is a list of whishes for change not bugs :s, at least point 3.
But I saw you post here http://www.sencha.com/forum/showthread.php?152655-Ext.data.Batch-should-obey-synchronous-setting-for-an-Ext.data.Operation and agree with you.


OT:
And about the "loader" during write communication.
I have a "bug" registered from my users when they wait for update to complete. They do not know if clicking "save" button is processing or not. Solution was quite simple - add an appropriate info in beforesync event.
But, sometimes things are annoying, sometimes not ;). The best thing is to have an OPTION to do something. And that is direction where commercial library should go.