Fix double saving snapshots (#1937)
* Fix double saving snapshots https://forums.zotero.org/discussion/86796/duplicated-snapshots I was able to replicate it by adding a 5 second delay here:a72ae14816/chrome/content/zotero/xpcom/translation/translate_item.js (L196)This was caused from a race condition and (a72ae14) did not fully solve the problem. Now this is tested and fixed.
This commit is contained in:
fletcherhaz
GitHub
parent
460be2a8c3
commit
98a75931b0
2 changed files with 175 additions and 8 deletions
|
|
@ -270,8 +270,6 @@ Zotero.Server.Connector.SaveSession.prototype.update = async function (targetID,
|
|||
for (let item of this._items) {
|
||||
await item.eraseTx();
|
||||
}
|
||||
// Remove pending attachments (will be recreated by calling `save...` below)
|
||||
this.pendingAttachments = [];
|
||||
let actionUC = Zotero.Utilities.capitalize(this._action);
|
||||
// saveItems has a different signature with the session as the first argument
|
||||
let params = [targetID, this._requestData];
|
||||
|
|
@ -855,7 +853,7 @@ Zotero.Server.Connector.SaveItems.prototype = {
|
|||
cookieSandbox,
|
||||
proxy
|
||||
});
|
||||
// This is a bit tricky. When saving items, the call back`onTopLevelItemsDone` will
|
||||
// This is a bit tricky. When saving items, the callback `onTopLevelItemsDone` will
|
||||
// return the HTTP request to the connector. Then it may spend some time fetching
|
||||
// PDFs. In the meantime, the connector will create a snapshot and send it along to
|
||||
// the `saveSingleFile` endpoint, which quickly adds the data to the session and
|
||||
|
|
@ -863,22 +861,27 @@ Zotero.Server.Connector.SaveItems.prototype = {
|
|||
// the session switches libraries and we need to save again). So the pending
|
||||
// attachments exist and have already been saved by the time this `saveItems`
|
||||
// promise resolves and we continue executing. So we save the number of existing
|
||||
// attachments before that so prevent double saving.
|
||||
let hasPendingAttachments;
|
||||
// attachments before that to prevent double saving.
|
||||
let hadPendingAttachments = session.pendingAttachments.length > 0;
|
||||
if (hadPendingAttachments) {
|
||||
// If we have pending attachments then we are saving again by switching to
|
||||
// a `filesEditable` library. So we clear the pendingAttachments since they
|
||||
// get added again right below here in `saveItems`
|
||||
session.pendingAttachments = [];
|
||||
}
|
||||
let items = await itemSaver.saveItems(
|
||||
data.items,
|
||||
function (attachment, progress, error) {
|
||||
session.onProgress(attachment, progress, error);
|
||||
},
|
||||
(...args) => {
|
||||
hasPendingAttachments = session.pendingAttachments.length > 0;
|
||||
if (onTopLevelItemsDone) onTopLevelItemsDone(...args);
|
||||
},
|
||||
function (parentItemID, attachment) {
|
||||
session.pendingAttachments.push([parentItemID, attachment]);
|
||||
}
|
||||
);
|
||||
if (hasPendingAttachments) {
|
||||
if (hadPendingAttachments) {
|
||||
// If the session has snapshotContent already (from switching to a `filesEditable` library
|
||||
// then we can save `pendingAttachments` now
|
||||
if (data.snapshotContent) {
|
||||
|
|
@ -1220,7 +1223,9 @@ Zotero.Server.Connector.SaveSnapshot.prototype = {
|
|||
// pending attachment
|
||||
else if (data.hasOwnProperty('singleFile')) {
|
||||
let session = Zotero.Server.Connector.SessionManager.get(data.sessionID);
|
||||
session.pendingAttachments.push([itemID, { title: data.title, url: data.url }]);
|
||||
session.pendingAttachments = [
|
||||
[itemID, { title: data.title, url: data.url }]
|
||||
];
|
||||
}
|
||||
else if (library.filesEditable) {
|
||||
// Old connector will not use SingleFile so importFromURL now
|
||||
|
|
|
|||
|
|
@ -1118,6 +1118,168 @@ describe("Connector Server", function () {
|
|||
let contents = await Zotero.File.getContentsAsync(path);
|
||||
assert.match(contents, /^<html style><!--\n Page saved with SingleFile \n url:/);
|
||||
});
|
||||
|
||||
it("should handle race condition with /saveItems", async function () {
|
||||
let collection = await createDataObject('collection');
|
||||
await waitForItemsLoad(win);
|
||||
|
||||
let pdfURL = testServerPath + '/pdf';
|
||||
let nonOADOI = '10.2222/bcde';
|
||||
|
||||
// Promise for item saving
|
||||
let parentIDs, attachmentIDs1, attachmentIDs2;
|
||||
let promise = waitForItemEvent('add').then(function (ids) {
|
||||
parentIDs = ids;
|
||||
return waitForItemEvent('add').then(function (ids) {
|
||||
attachmentIDs1 = ids;
|
||||
return waitForItemEvent('add').then(function (ids) {
|
||||
attachmentIDs2 = ids;
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// Promise for snapshot having been saved
|
||||
let singleFileResolve;
|
||||
let singleFileDone = new Zotero.Promise(function (resolve, reject) {
|
||||
singleFileResolve = resolve;
|
||||
});
|
||||
|
||||
// Special handler to delay writing of file response for 5 seconds to allow
|
||||
// `saveSingleFile` request to finish first before getting PDF
|
||||
httpd.registerPathHandler(
|
||||
'/pdf',
|
||||
{
|
||||
handle: async function (request, response) {
|
||||
response.setStatusLine(null, 200, "OK");
|
||||
let file = Zotero.File.pathToFile(OS.Path.join(getTestDataDirectory().path, 'test.pdf'));
|
||||
response.processAsync();
|
||||
// Delay the PDF processing (simulates a long network request) so that
|
||||
// the SingleFile request below completes first.
|
||||
await singleFileDone;
|
||||
httpd._handler._writeFileResponse(request, file, response, 0, file.fileSize);
|
||||
}
|
||||
}
|
||||
);
|
||||
|
||||
// Setup our `saveItems` and payload and call connector server
|
||||
let title = Zotero.Utilities.randomString();
|
||||
let sessionID = Zotero.Utilities.randomString();
|
||||
let payload = {
|
||||
sessionID,
|
||||
items: [
|
||||
{
|
||||
itemType: 'journalArticle',
|
||||
title: title,
|
||||
DOI: nonOADOI,
|
||||
attachments: [
|
||||
{
|
||||
title: "PDF",
|
||||
url: pdfURL,
|
||||
mimeType: 'application/pdf'
|
||||
},
|
||||
{
|
||||
title: "Snapshot",
|
||||
url: `${testServerPath}/attachment`,
|
||||
mimeType: "text/html",
|
||||
singleFile: true
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
uri: "http://example.com"
|
||||
};
|
||||
|
||||
let req = await Zotero.HTTP.request(
|
||||
'POST',
|
||||
connectorServerPath + "/connector/saveItems",
|
||||
{
|
||||
headers: {
|
||||
"Content-Type": "application/json"
|
||||
},
|
||||
body: JSON.stringify(payload)
|
||||
}
|
||||
);
|
||||
assert.equal(req.status, 201);
|
||||
|
||||
// Now setup and call our `saveSingleFile` to save snapshot attachment
|
||||
let testDataDirectory = getTestDataDirectory().path;
|
||||
let indexPath = OS.Path.join(testDataDirectory, 'snapshot', 'index.html');
|
||||
|
||||
let body = JSON.stringify(Object.assign(payload, {
|
||||
snapshotContent: await Zotero.File.getContentsAsync(indexPath)
|
||||
}));
|
||||
|
||||
req = await Zotero.HTTP.request(
|
||||
'POST',
|
||||
connectorServerPath + "/connector/saveSingleFile",
|
||||
{
|
||||
headers: {
|
||||
"Content-Type": "application/json"
|
||||
},
|
||||
body
|
||||
}
|
||||
);
|
||||
assert.equal(req.status, 201);
|
||||
|
||||
// Trigger PDF saving to complete now that SingleFile is done.
|
||||
singleFileResolve();
|
||||
|
||||
// Await all item saves
|
||||
await promise;
|
||||
|
||||
// Once the PDF is saved, if the bug exists, the snapshot will saved again.
|
||||
// Once that is completed, then the session will be marked as done so we
|
||||
// wait for that to occur here. Then we can proceed to ensure we have the
|
||||
// proper number of items.
|
||||
let savingDone = false;
|
||||
while (!savingDone) {
|
||||
// eslint-disable-next-line no-await-in-loop
|
||||
req = await Zotero.HTTP.request(
|
||||
'POST',
|
||||
connectorServerPath + "/connector/sessionProgress",
|
||||
{
|
||||
headers: {
|
||||
"Content-Type": "application/json"
|
||||
},
|
||||
body: JSON.stringify({ sessionID })
|
||||
}
|
||||
);
|
||||
savingDone = JSON.parse(req.response).done;
|
||||
if (!savingDone) {
|
||||
// eslint-disable-next-line no-await-in-loop
|
||||
await Zotero.Promise.delay(1000);
|
||||
}
|
||||
}
|
||||
|
||||
// Check parent item
|
||||
assert.lengthOf(parentIDs, 1);
|
||||
let item = Zotero.Items.get(parentIDs[0]);
|
||||
assert.equal(Zotero.ItemTypes.getName(item.itemTypeID), 'journalArticle');
|
||||
assert.isTrue(collection.hasItem(item.id));
|
||||
|
||||
// Ensure we only have one snapshot and one PDF - this is the critical test
|
||||
assert.equal(item.numChildren(), 2);
|
||||
|
||||
// Snapshot is saved first
|
||||
assert.lengthOf(attachmentIDs1, 1);
|
||||
item = Zotero.Items.get(attachmentIDs1[0]);
|
||||
assert.isTrue(item.isImportedAttachment());
|
||||
assert.equal(item.getField('title'), 'Snapshot');
|
||||
|
||||
// Double check snapshot html file has content
|
||||
let attachmentDirectory = Zotero.Attachments.getStorageDirectory(item).path;
|
||||
let path = OS.Path.join(attachmentDirectory, 'attachment.html');
|
||||
assert.isTrue(await OS.File.exists(path));
|
||||
let contents = await Zotero.File.getContentsAsync(path);
|
||||
let expectedContents = await Zotero.File.getContentsAsync(indexPath);
|
||||
assert.equal(contents, expectedContents);
|
||||
|
||||
// Then PDF is saved second
|
||||
assert.lengthOf(attachmentIDs2, 1);
|
||||
item = Zotero.Items.get(attachmentIDs2[0]);
|
||||
assert.isTrue(item.isImportedAttachment());
|
||||
assert.equal(item.getField('title'), 'PDF');
|
||||
});
|
||||
});
|
||||
|
||||
describe("/connector/saveSnapshot", function () {
|
||||
|
|
|
|||
Loading…
Reference in a new issue