Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#6562 closed defect (fixed)

ItemFileWriteStore fails if fetchByIdentity is called before newItem

Reported by: guest Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.2
Component: Data Version: 1.1.0
Keywords: Cc:
Blocked By: Blocking:

Description

I've attached a test that fails if one calls fetchByIdentity on a ItemFileWriteStore? before newItem.

It fails with: Error: newItem() was not passed an identity for the new item

ERROR IN:

(function testWriteAPI_fetchThenNewItem() {var store = new dojo.data.ItemFileWriteStore?(tests.data.readOnlyItemFileTestTemplates.getTestData("countries"));var deferred = new doh.Deferred;doh.assertTrue(!store.isDirty()); function onItem(item, parentInfo) {doh.assertTrue(item !== null);doh.assertTrue(parentInfo === null);doh.assertTrue(store.isItem(item));deferred.callback(true);} function onError(error, request) {deferred.errback(error);}

This is running against trunk.

Attachments (4)

ssm-dojo-bug.diff (1.7 KB) - added by guest 11 years ago.
bug-6562-test.diff (1.7 KB) - added by guest 11 years ago.
updated diff
dojo.data_fetchThenNewItemTest.patch (1.8 KB) - added by Jared Jurkiewicz 11 years ago.
Fixed patch that corrects two errors made by the testcases provided in the users diffs.
dojo.data.ItemFileStore_concurrentyCollision_20080505.patch (1.1 KB) - added by Jared Jurkiewicz 11 years ago.
Patch to throw a more meaningful error on a load collision.

Download all attachments as: .zip

Change History (18)

Changed 11 years ago by guest

Attachment: ssm-dojo-bug.diff added

comment:1 Changed 11 years ago by bill

Component: GeneralData
Owner: changed from anonymous to Jared Jurkiewicz
Summary: ItemFileWriteStore fails in fetchByIdentity is called before newItemItemFileWriteStore fails if fetchByIdentity is called before newItem

Changed 11 years ago by guest

Attachment: bug-6562-test.diff added

updated diff

comment:2 Changed 11 years ago by guest

I borked my original diff somehow so I uploaded a better one.

comment:3 Changed 11 years ago by Jared Jurkiewicz

I'm not sure this is fixable. The problem stems from async versus non-async loading. The first fetch invokes the load and post-processing of the dataset. That needs to complete before other events can operate on the ItemFileWriteStore?. All the async methods queue off requests that occur while a load is in progress (since they're all deferred this is doable relatively easily. However, since newItem isn't async and there's no javascript blocking wait (no thread conditionals), there's no good way to paust a newItem to wait for the fetch to return and the set of data to complete loading. Will see if I can come up with a solution for it, but for this particular datastore, there may not be one.

comment:4 Changed 11 years ago by Jared Jurkiewicz

Also, minor nit in the patch, you should return the deferred instance, not thje doh instance, if you want async tests to properly work.

comment:5 Changed 11 years ago by Jared Jurkiewicz

Haven't been able to reproduce your error, though I did produce an error with the unit test you provided. The assertTrue check on parentInfo wasn't right. You need to verify it's not null or undefined. Generally, the value is undefined because it passes the parentInfo object straight from newItem to the onNew call, so if you didn't provide one, it doesn't pass anything on (undefined param). ItemFileWriteStore? already tries to force a load if the load hasn't occured yet, so it's doing its best to ensure the load flow is always done before newItem does anything against the store. So, I'm not sure you're hitting that as the problem, but ... I'll need more info on your exact run environment, as I can't reproduce it.

What browser?

Where is dojo run? (Local filesystem or http server? I tried both and couldn't reproduce)

Results from my test below, with dojo 1.1 running on an Apache server: (0ms)bootstrap.js (line 1000) GET http://localhost:8080/dojo11/dojo/tests/data/countries.json (16ms)bootstrap.js (line 1000) PASSED test: testWriteAPI_fetchThenNewItembootstrap.js (line 485) GET http://localhost:8080/dojo11/dojo/tests/data/countries.json (0ms)

Trunk running on an apache server: GET http://localhost:8080/dojoTest/dojo/tests/data/countries.json (16ms)bootstrap.js (line 1000) PASSED test: testWriteAPI_fetchThenNewItembootstrap.js (line 485) GET http://localhost:8080/dojoTest/dojo/tests/data/countries.json (0ms)

More correct UT testcase will be attached as a patch to this.

Changed 11 years ago by Jared Jurkiewicz

Fixed patch that corrects two errors made by the testcases provided in the users diffs.

comment:6 Changed 11 years ago by Jared Jurkiewicz

Resolution: worksforme
Status: newclosed

Unable to reproduce with corrected testcase. Please try the fixed testcase and if it still fails for you, reopen with more informagtion, such as browser, where the test was loaded from, etc.

comment:7 Changed 11 years ago by Jared Jurkiewicz

Resolution: worksforme
Status: closedreopened

Actually reproduced this. Only occurs on Safari. Firefox, IE, Opera, are all fine.

Looking at possible solutions. Not quite sure how to exactly fix it, given there are no javascript 'sleep' functions to block execution.

comment:8 Changed 11 years ago by Jared Jurkiewicz

For some reason on Safari only, async xhr gets delayed a lot if a sync xhr is also called. Not sure why this is, but I can see it happening. Not sure how to fix it yet, either (if it's even possible). On IE/Firefox/Opera, this works because the async xhr always completes and gets callback before the sync.

comment:9 Changed 11 years ago by guest

I applied your patch and the the testWriteAPI_fetchThenNewItem test still fails in the same way:

    Error: newItem() was not passed an identity for the new item

I tried running this locally and on http server in Firefox 2.0.0.13. Running on trunk.

Running opera 9.27 all tests pass. Fun!

comment:10 Changed 11 years ago by Jared Jurkiewicz

Yes, it's 'fun'. :-) I'm not sure how to address it yet, given JavaScript?'s inability to block a thread of execution. That's really what is needed here; I need to just halt newItem until the first fetch finishes (and the load has completed).

A workaround for it is to put your newItem calls inside your onItem handler of the fetchItemByIdentity(). The main thing is, the fetch needs to finish before you call newItem, otherwise the load state ends up inconsistent.

comment:11 Changed 11 years ago by Jared Jurkiewicz

I think the best I can manage here is to throw a more understandable error that a concurrency collision occurred and that the synchronous operation (newItem), cannot be completed at this time. At least it would be more obvious what happened and that the fetch just needs to complete.

Will work up a patch for a reasonable error message and commit it as the correction for this issue.

Changed 11 years ago by Jared Jurkiewicz

Patch to throw a more meaningful error on a load collision.

comment:12 Changed 11 years ago by Jared Jurkiewicz

Resolution: fixed
Status: reopenedclosed

Fixed this with a better error message in 1.2. Revision: 13571.

comment:13 Changed 11 years ago by Jared Jurkiewicz

(In [13580]) Tweak to message. refs #6562 \!strict

comment:14 Changed 11 years ago by bill

Milestone: 1.2

marking tickets closed in the last three months w/blank milestone to milestone 1.2.

Note: See TracTickets for help on using tickets.