Skip to content
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

createinstance and updateinstance in BaseConnection::save are not atomic. #5518

Closed
qantourisc opened this issue Aug 4, 2021 · 6 comments
Closed

Comments

@qantourisc
Copy link

When createInstance or updateInstance is called in "save: function(instance){}" they are not batched.
As such if you have an observation on an object-instance, you can get multiple calls for each key/member/field/prop.

Fictional example case:
Assume an Object that has to following fields: "GUID, ID, name". To create a new object you only submit "Name" to the server. The server then replies with {"GUID":"0F5581" , "ID":1, name: "Name}. If you now create an observation of that object on GUID and ID, your observation will be trigger twice. If you then required both a valid GUI and ID at that point in time, you will get an error since ID hasn't been updated yet.

Consider adding stop and start batch in these positions.
(I needed to add this for my GUI code to work, the update/creation had to be atomic from the point of the observer.)

@justinbmeyer
Copy link
Contributor

justinbmeyer commented Aug 4, 2021

By batched, you mean the properties being set are not batched together? What do you mean "multiple calls"?

Afaik, setting multiple properties to a can.Map and can.DefineMap are batched automatically.

Could you share an example of what you are seeing?

@qantourisc
Copy link
Author

By batched, you mean the properties being set are not batched together? <= yes.

What do you mean "multiple calls"?

import { ObservableObject, Observation } from "//unpkg.com/can@6/everything.mjs";

class Example extends ObservableObject{
	save(){
		//Normally this would be done by the baseConnection behaviour
		//And the updateInstance and createInstance functions
		this.id=0;
		this.name="test";
	}
}

var tmp = new Example;
var obs = new Observation(()=>{
	return [tmp.id, tmp.name];
});

obs.on((a)=>{console.log(a);})

tmp.save();

To fix the example above add batch start/stop. And it would only be printed once.

Afaik, setting multiple properties to a can.Map and can.DefineMap are batched automatically.
-> BaseConnection::save -> BaseConnection::createInstance -> assign$2 -> canReflect_1_18_0_canReflect.assignMap -> shapeReflections.eachKey(source,function(value, key){

I'm not sure I followed that code correctly, but that would update it 1 by 1 without a batch.

I hope I explained myself better / clearly. If not i'll try again with a more complete example.

@justinbmeyer
Copy link
Contributor

We are taking a look at this. Thanks for reporting!

@qantourisc
Copy link
Author

FYI the patch is short, should I make a pull request ?

@@ -35364,7 +35370,9 @@
 				return this.createData(serialized, cid).then(function(data){
 					// if undefined is returned, this can't be created, or someone has taken care of it
 					if(data !== undefined) {
+						canQueues_1_3_2_canQueues.batch.start();
 						self.createdInstance(instance, data);
+						canQueues_1_3_2_canQueues.batch.stop();
 					}
 					self.cidStore.deleteReference(cid, instance);
 					return instance;
@@ -35372,7 +35380,9 @@
 			} else {
 				return this.updateData(serialized).then(function(data){
 					if(data !== undefined) {
+						canQueues_1_3_2_canQueues.batch.start();
 						self.updatedInstance(instance, data);
+						canQueues_1_3_2_canQueues.batch.stop();
 					}
 					return instance;
 				});

@justinbmeyer
Copy link
Contributor

Sorry about the long delay. Yes, please make a patch!

@qantourisc
Copy link
Author

I see you added this to 6.6.3 (or earlier) thank you !
Closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants