From 9b705a6375d83c43ff21607e32c846c92b9080ac Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Tue, 14 Nov 2017 21:30:01 -0500 Subject: [PATCH] Don't call async callbacks after connection resets/switches (closes #4288) --- modules/core/context.js | 34 +++++++----- modules/renderer/map.js | 8 +-- modules/services/osm.js | 104 ++++++++++++++++++++++++++++-------- modules/ui/source_switch.js | 18 ++++--- modules/ui/status.js | 7 ++- test/spec/services/osm.js | 15 ++++++ 6 files changed, 139 insertions(+), 47 deletions(-) diff --git a/modules/core/context.js b/modules/core/context.js index 293b8e19d..3eb716a7d 100644 --- a/modules/core/context.js +++ b/modules/core/context.js @@ -112,10 +112,6 @@ export function coreContext() { /* Connection */ - function entitiesLoaded(err, result) { - if (!err) history.merge(result.data, result.extent); - } - context.preauth = function(options) { if (connection) { connection.switch(options); @@ -124,39 +120,51 @@ export function coreContext() { }; context.loadTiles = utilCallWhenIdle(function(projection, dimensions, callback) { + var cid; function done(err, result) { - entitiesLoaded(err, result); + if (connection.getConnectionId() !== cid) { + if (callback) callback({ message: 'Connection Switched', status: -1 }); + return; + } + if (!err) history.merge(result.data, result.extent); if (callback) callback(err, result); } if (connection) { + cid = connection.getConnectionId(); connection.loadTiles(projection, dimensions, done); } }); - context.loadEntity = function(id, callback) { + context.loadEntity = function(entityId, callback) { + var cid; function done(err, result) { - entitiesLoaded(err, result); + if (connection.getConnectionId() !== cid) { + if (callback) callback({ message: 'Connection Switched', status: -1 }); + return; + } + if (!err) history.merge(result.data, result.extent); if (callback) callback(err, result); } if (connection) { - connection.loadEntity(id, done); + cid = connection.getConnectionId(); + connection.loadEntity(entityId, done); } }; - context.zoomToEntity = function(id, zoomTo) { + context.zoomToEntity = function(entityId, zoomTo) { if (zoomTo !== false) { - this.loadEntity(id, function(err, result) { + this.loadEntity(entityId, function(err, result) { if (err) return; - var entity = _find(result.data, function(e) { return e.id === id; }); + var entity = _find(result.data, function(e) { return e.id === entityId; }); if (entity) { map.zoomTo(entity); } }); } map.on('drawn.zoomToEntity', function() { - if (!context.hasEntity(id)) return; + if (!context.hasEntity(entityId)) return; map.on('drawn.zoomToEntity', null); context.on('enter.zoomToEntity', null); - context.enter(modeSelect(context, [id])); + context.enter(modeSelect(context, [entityId])); }); context.on('enter.zoomToEntity', function() { diff --git a/modules/renderer/map.js b/modules/renderer/map.js index 56dbbf8d1..de88fb343 100644 --- a/modules/renderer/map.js +++ b/modules/renderer/map.js @@ -74,10 +74,10 @@ export function rendererMap(context) { mousemove; var zoom = d3_zoom() - .scaleExtent([ztok(2), ztok(24)]) - .interpolate(d3_interpolate) - .filter(zoomEventFilter) - .on('zoom', zoomPan); + .scaleExtent([ztok(2), ztok(24)]) + .interpolate(d3_interpolate) + .filter(zoomEventFilter) + .on('zoom', zoomPan); var _selection = d3_select(null); var isRedrawScheduled = false; diff --git a/modules/services/osm.js b/modules/services/osm.js index 94e59207b..cf33659a3 100644 --- a/modules/services/osm.js +++ b/modules/services/osm.js @@ -31,6 +31,7 @@ var dispatch = d3_dispatch('authLoading', 'authDone', 'change', 'loading', 'load inflight = {}, loadedTiles = {}, entityCache = {}, + connectionId = 1, tileZoom = 16, oauth = osmAuth({ url: urlroot, @@ -189,6 +190,7 @@ export default { reset: function() { + connectionId++; userChangesets = undefined; userDetails = undefined; rateLimitError = undefined; @@ -200,6 +202,11 @@ export default { }, + getConnectionId: function() { + return connectionId; + }, + + changesetURL: function(changesetId) { return urlroot + '/changeset/' + changesetId; }, @@ -232,8 +239,14 @@ export default { loadFromAPI: function(path, callback, options) { options = _extend({ cache: true }, options); var that = this; + var cid = connectionId; function done(err, xml) { + if (that.getConnectionId() !== cid) { + if (callback) callback({ message: 'Connection Switched', status: -1 }); + return; + } + var isAuthenticated = that.authenticated(); // 400 Bad Request, 401 Unauthorized, 403 Forbidden @@ -333,6 +346,8 @@ export default { putChangeset: function(changeset, changes, callback) { + var that = this; + var cid = connectionId; // Create the changeset.. oauth.xhr({ @@ -344,7 +359,13 @@ export default { function createdChangeset(err, changeset_id) { - if (err) return callback(err); + if (err) { + return callback(err); + } + if (that.getConnectionId() !== cid) { + return callback({ message: 'Connection Switched', status: -1 }); + } + changeset = changeset.update({ id: changeset_id }); // Upload the changeset.. @@ -366,12 +387,16 @@ export default { callback(null, changeset); }, 2500); - // Still attempt to close changeset, but ignore response because #2667 - oauth.xhr({ - method: 'PUT', - path: '/api/0.6/changeset/' + changeset.id + '/close', - options: { header: { 'Content-Type': 'text/xml' } } - }, function() { return true; }); + // At this point, we don't really care if the connection was switched.. + // Only try to close the changeset if we're still talking to the same server. + if (that.getConnectionId() === cid) { + // Still attempt to close changeset, but ignore response because #2667 + oauth.xhr({ + method: 'PUT', + path: '/api/0.6/changeset/' + changeset.id + '/close', + options: { header: { 'Content-Type': 'text/xml' } } + }, function() { return true; }); + } } }, @@ -382,8 +407,16 @@ export default { return; } + var that = this; + var cid = connectionId; + function done(err, user_details) { - if (err) return callback(err); + if (err) { + return callback(err); + } + if (that.getConnectionId() !== cid) { + return callback({ message: 'Connection Switched', status: -1 }); + } var u = user_details.getElementsByTagName('user')[0], img = u.getElementsByTagName('img'), @@ -420,27 +453,36 @@ export default { return; } + var that = this; + var cid = connectionId; + this.userDetails(function(err, user) { if (err) { - callback(err); - return; + return callback(err); + } + if (that.getConnectionId() !== cid) { + return callback({ message: 'Connection Switched', status: -1 }); } function done(err, changesets) { if (err) { - callback(err); - } else { - userChangesets = Array.prototype.map.call( - changesets.getElementsByTagName('changeset'), - function (changeset) { - return { tags: getTags(changeset) }; - } - ).filter(function (changeset) { - var comment = changeset.tags.comment; - return comment && comment !== ''; - }); - callback(undefined, userChangesets); + return callback(err); } + if (that.getConnectionId() !== cid) { + return callback({ message: 'Connection Switched', status: -1 }); + } + + userChangesets = Array.prototype.map.call( + changesets.getElementsByTagName('changeset'), + function (changeset) { + return { tags: getTags(changeset) }; + } + ).filter(function (changeset) { + var comment = changeset.tags.comment; + return comment && comment !== ''; + }); + + callback(undefined, userChangesets); } oauth.xhr({ method: 'GET', path: '/api/0.6/changesets?user=' + user.id }, done); @@ -449,7 +491,14 @@ export default { status: function(callback) { + var that = this; + var cid = connectionId; + function done(xml) { + if (that.getConnectionId() !== cid) { + return callback({ message: 'Connection Switched', status: -1 }, 'connectionSwitched'); + } + // update blacklists var elements = xml.getElementsByTagName('blacklist'), regexes = []; @@ -568,9 +617,9 @@ export default { done: authDone }, options)); - dispatch.call('change'); this.reset(); this.userChangesets(function() {}); // eagerly load user details/changesets + dispatch.call('change'); return this; }, @@ -599,10 +648,19 @@ export default { authenticate: function(callback) { var that = this; + var cid = connectionId; userChangesets = undefined; userDetails = undefined; function done(err, res) { + if (err) { + if (callback) callback(err); + return; + } + if (that.getConnectionId() !== cid) { + if (callback) callback({ message: 'Connection Switched', status: -1 }); + return; + } rateLimitError = undefined; dispatch.call('change'); if (callback) callback(err, res); diff --git a/modules/ui/source_switch.js b/modules/ui/source_switch.js index 475cd4801..fe54f5a67 100644 --- a/modules/ui/source_switch.js +++ b/modules/ui/source_switch.js @@ -13,22 +13,28 @@ export function uiSourceSwitch(context) { function click() { d3_event.preventDefault(); + + var osm = context.connection(); + if (!osm) return; + if (context.inIntro()) return; if (context.history().hasChanges() && !window.confirm(t('source_switch.lose_changes'))) return; - var live = d3_select(this) + var isLive = d3_select(this) .classed('live'); - context.history().clearSaved(); - context.connection().switch(live ? keys[1] : keys[0]); + isLive = !isLive; context.enter(modeBrowse(context)); - context.flush(); + context.history().clearSaved(); // remove saved history + context.flush(); // remove stored data d3_select(this) - .text(live ? t('source_switch.dev') : t('source_switch.live')) - .classed('live', !live); + .text(isLive ? t('source_switch.live') : t('source_switch.dev')) + .classed('live', isLive); + + osm.switch(isLive ? keys[0] : keys[1]); // switch connection (warning: dispatches 'change' event) } var sourceSwitch = function(selection) { diff --git a/modules/ui/status.js b/modules/ui/status.js index ec758011c..0dc62b4c2 100644 --- a/modules/ui/status.js +++ b/modules/ui/status.js @@ -16,7 +16,12 @@ export function uiStatus(context) { selection.html(''); if (err) { - if (apiStatus === 'rateLimited') { + if (apiStatus === 'connectionSwitched') { + // if the connection was just switched, we can't rely on + // the status (we're getting the status of the previous api) + return; + + } else if (apiStatus === 'rateLimited') { selection .text(t('status.rateLimit')) .append('a') diff --git a/test/spec/services/osm.js b/test/spec/services/osm.js index a38c69bf3..3f51916f1 100644 --- a/test/spec/services/osm.js +++ b/test/spec/services/osm.js @@ -52,6 +52,21 @@ describe('iD.serviceOsm', function () { expect(connection.changesetURL(2)).to.match(/^https:/); }); + describe('#getConnectionId', function() { + it('changes the connection id every time connection is reset', function() { + var cid1 = connection.getConnectionId(); + connection.reset(); + var cid2 = connection.getConnectionId(); + expect(cid2).to.be.above(cid1); + }); + it('changes the connection id every time connection is switched', function() { + var cid1 = connection.getConnectionId(); + connection.switch({ urlroot: 'https://api06.dev.openstreetmap.org' }); + var cid2 = connection.getConnectionId(); + expect(cid2).to.be.above(cid1); + }); + }); + describe('#changesetURL', function() { it('provides a changeset url', function() { expect(connection.changesetURL(2)).to.eql('http://www.openstreetmap.org/changeset/2');