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

Duplicate incident tabs is bad #92

Closed
wsanchez opened this issue Jul 31, 2016 · 14 comments
Closed

Duplicate incident tabs is bad #92

wsanchez opened this issue Jul 31, 2016 · 14 comments
Assignees
Labels

Comments

@wsanchez
Copy link
Member

Duplicate incident tabs is bad because out of sync.

Is it possible to prevent this?

@wsanchez wsanchez added the Bug label Jul 31, 2016
@wsanchez wsanchez added this to the Burning Man 2016 milestone Jul 31, 2016
@kimballa
Copy link

Yes, using HTML5 Local Storage (see e.g. http://www.w3schools.com/html/html5_webstorage.asp)

Basically, you can have a cache of incidentIds -> incident JSON objects that is shared across tabs that are accessing the same domain. You can attach js event listeners on updates to the cache.

See e.g. the following regarding using event listeners

Note that this doesn't handle a (I assume related) problem: if my partner and I both have the incident open in a tab on each of our consoles, my updates will not be seen by her, nor vice versa, so we could clobber each other's state.

Example

Here's a sketch of an example update scenario for local tab sharing. First off, I haven't examined your code so am not familiar with the architecture you've set up.

I assume you've got a JS method (let's call it sendNewData()) that does an AJAX POST to the server with your latest complete state data, triggered by onFocusOut from an updateable data field.

Also, I apologize in advance but my jQuery and etc. is pretty rusty; I'm more of a Java guy. This probably has both syntax and logic errors but my javascript is very seat-of-the-pants flying.

This method can look something like this:

void sendNewData(event) {
  userState = {};
  userState["summary"] = $("#summary").value();
  ... // build a JSON object containing a complete view of the local state

  // Before waiting for a response from the server, immediately update the local cache with
  // our new data for this incident
  localStorage.setItem("incident#" + myIncidentId, userState);

  // Then send the server our view of the incident.
  $.post({
    url: "http://" + myServer + "/v1/incidents/" + myIncidentId, // or whatever the POST target is.
    data: userState, // send this json map as the POST payload
    success: handleAjaxUpdate, // a method that handles latest state from server
    error: handleError, // a method that handles a 401 or other err response from server
    ... // other args to the ajax post method
  });
}

/**
  * Re-render all fields that need rendering based on a complete view of an incident
  * @param incident a hash of data that represents the current incident.
 */
function updateWindowFromData(incident) {
  $("#summary").value(incident["summary"]); 
  // and set other local fields as appropriate.
}

/**
  * Handle a response from the server.
  * @param data a hash of data from the server that represents the current incident.
 */
function handleAjaxUpdate(data) {
  // set local fields based on the incoming data. The server takes precedence over local state.
  updateWindowFromData(data);

  // finally, since the server's word is law, update our local cache.
  localStorage.setItem("incident#" + myIncidentId, data);
}

You need to add a listener to request a function fires when local storage is updated. Do something like this onLoad (e.g., in a jQuery initializer):

/** Make this method called onLoad */
function initPage() {
  // TODO: I think there's a better jquery-ish way to do this.
  window.addEventListener('storage', handleStorageUpdate, false);
}

/** Fired any time the localStorage is updated by any tab. */
function handleStorageUpdate() {
  // Get our incident...
  cachedIncident = localStorage.getItem("incident#" + myIncidentId);
  // ...and update the view.
  updateWindowFromData(cachedIncident);  
}

@kimballa
Copy link

I think the above is actually a mild oversimplification; I'm pretty sure you can't actually use an Object (hash) as the data in localStorage.setItem(key, data). I think the data must be a string, so you need to serialize your object to a JSON string first.

Similarly, handleStorageUpdate() must deserialize from JSON to an object.

@kimballa
Copy link

kimballa commented Aug 1, 2016

Multi-User Synchronization

As I alluded to above, this won't handle the situation where Operators Hubcap and Bucket both have the same incident open in their terminal. For that, you'll need to

  • Client Side: Make sure you use partial updates, so you don't clobber fields whose state you do not intend to represent the most current world
  • Server side: support partial updates
  • Client side: poll (or long-poll listen) for updates from the server

As a disclaimer, I have no idea how the current code works, so apologies if some of this is redundant to your current implementation. I honestly haven't looked at it, which is a dangerous place to propose solution implementations from ;)

Partial Updates on the client side

Your update method(s) that are fired onFocusOut from a field should only send back the specific field that was modified.

e.g.

function updateSummary() {
  data = {}
  data["id"] = myIncidentId;
  data["summary"] = $("#summary").value();
  $.ajax({  // Use $.ajax, not $.post to control http method.
    method: "PATCH", // Use PATCH not POST to, in a REST-ful way, indicate that you are sending
                                  // a partial update and not a complete object representation.
    data: data, // Use our data hash created above: { id: 123, summary: "Something bad happened" }
    success: handleAjaxUpdate, // Use method from above; server responds w/ complete object
    error: handleError, // Handle a 4xx or 5xx response from server
    ... // other arguments as required
  });
}

// And add methods like addNewRanger(), addIncidentType(), removeIncidentType(), 
// addComment(), etc as required.

Server-side updates

Left as an exercise to the reader. Basically, don't persist the incoming object literally, you must
overlay its state changes onto the database source-of-truth version of the incident.

This should rely on the HTTP PATCH method, not POST, since it's a partial update. Since a partial
update can contain multiple fields (or even all fields), a method written in "overlay style" can
also respond to POST.

This should return the complete source-of-truth view of the incident to the caller as a json object.

Polling for updates

Hubcap sends an update. How does Bucket get it?

In a jQuery initializer or other method fired onLoad, you should initialize a recurring timer (using setInterval()) that does an AJAX GET. e.g., every two seconds (2000 ms), it should call something like:

function updateTimerHandler() {
  $.ajax({
    method: "GET", 
    url: "http://" + myServer + "/v1/incidents/" + myIncidentId, // n.b., https is better.
    success: handleAjaxUpdate, // as before 
    error: handleError, // as before...
    ... // other args.
}

Handling race conditions

At this point, there may be a race between server-side data input and client-side data input. For bonus points, the localStorage could contain a hash containing the current incident AND the timestamp at which that incident object was created. You should update the localStorage iff your new incident's timestamp is greater than the timestamp associated with the incident currently in storage.

Final thoughts

  • If the user has multiple tabs open for different incidents, putting any one incident to the localStorage will cause a storage-changed event for all tabs. You should use this same timestamp mechanism described above in conjunction with a lastUpdatedAt timestamp for the current tab to determine whether your view is dirty. Clean views should not be updated, otherwise you may clobber local user changes in progress.
  • All threads share a common view of the current timestamp.
  • You should only rely on client-side timestamps in the client (or only server-side timestamps), unless you're very certain of your ntp configuration. And even then... note that the lastUpdatedAt ts described above will be a client-side timestamp.
  • If a field is focused and has changed from its previously js-set value, you may not want to clobber its value() with data from another tab, since that would clobber current user entry.
  • Note that all events in each particular tab (indeed, all javascript functions for that tab) are executed in a single thread. Events are enqueued at the end of the current set of events-to-fire. AJAX response is an event, as is onLoad, onFocusOut, onChange, etc.
  • Different tabs have different threads, and localStorage is not synchronized. If you programmatically update the state from multiple threads, you may have read-modify-write races.
    • Since updates are mostly coming from user input activity, your storage change handlers should happen faster than a user can switch tabs and generate new input or focus-change events.
    • Multiple tabs that each poll the server for the same incident are not advised for this reason. You can have a single tab do the ajax polling, and rely on the localStorage changed handler to propagate changes to the other tabs. This requires that you perform leader election across your tabs, and each tab needs an interval timer that checks to make sure that a new leader election doesn't need to be performed (e.g., because you closed the tab containing the current leader). This is left as an exercise to the reader ;)
    • Since we take the server's view of the world as source-of-truth and a complete update to the view, it may be the case that this is a benign race that you can let slide here. Testing is advised.

@flwyd
Copy link

flwyd commented Aug 1, 2016

Other approaches:

  • Include a consistency token in server responses and client mutates.
    The consistency token is an arbitrary string, but could easily be a server timestamp.
    When a client sends an update, it includes a consistencyToken parameter. The server rejects the update if the consistency token doesn't match the currently stored value for that object.
    This would let you continue to do full-object updates (POST rather than PATCH) at the cost of requiring a data refresh if two people are editing the same incident.
  • Make updates idempotent.
    Rather than specifying just the new values for updated fields, like title: 'Dumpster fire', location: 'DPW Depot', make it more like a delta: changes: {title: {old: 'Fire', new: 'Dumpster fire'}, location: {old: 'DPW Ghetto', new: 'DPW Depot'}}
    When the server gets an update, it checks if the old value matches the one currently in the database. If not, it makes no changes, and indicates in the response that the field has already changed.

Both of these approaches work equally well for multiple tabs in the same browser or multiple users on different browsers. In either case you can poll periodically if you want to warn users in advance, but polling isn't necessary in either case.

@wsanchez
Copy link
Member Author

wsanchez commented Aug 1, 2016

@kimballa Thanks for all the references, that will help me ramp up on a few key things. Note the server doesn't generate events yet, but thats #46.

What I had intended for this ticket to be for, though, is that if you click on incident 43 in the dispatch queue, which opens a tab, and then click it again, what I'd like is that to that take you to the already open tab instead of creating a new tab for incident 43. I don't know how to do that.

@kimballa
Copy link

kimballa commented Aug 1, 2016

I did a quick google search for that one too, since it also seems like a useful improvement. That having been said, I also open new tabs and url-hack my way to things I need to see when I can't navigate a UI quickly enough, so please bear in mind that it will likely be impossible to prevent duplicate tabs in a client's browser :)

@kimballa
Copy link

kimballa commented Aug 1, 2016

@flwyd these are both good suggestions too. I think the main difference is the question "should stale data merging/patching be done client-side or server-side?" The logic required will be effectively the same, the question boils down to where to perform it.

From an idempotence standpoint,the consistency token could also be a hash (md5, sha1, etc) of the data payload, which is less sensitive to jitter than timestamps

@flwyd
Copy link

flwyd commented Aug 2, 2016

@wsanchez I think you're looking for window.open(url, name) with a name derived from the incident (like 'incident-' + incident.getId()).

Note that this means you have to use onclick handlers rather than letting actual links do their job, which can be annoying when someone wants to do actual linky things like right-click-copy or open two copies of an incident to compare old and new values. You might be able to use actual <a> tags and use event.preventDefault() to preserve some link behavior.

@flwyd
Copy link

flwyd commented Aug 2, 2016

Oh, wait. <a href="/incidents/42" target="incident-42">42</a> may do what you want and still be a useful link.

@wsanchez
Copy link
Member Author

wsanchez commented Aug 2, 2016

@flwyd: Oh, cool… I thought that only the reserved words (_self, _blank) etc. had to be used with target; it never dawned on me that you could label every context that way. Duh.

@wsanchez wsanchez self-assigned this Aug 2, 2016
@wsanchez
Copy link
Member Author

wsanchez commented Aug 2, 2016

OK, opened #94 and #95 for using local storage to cache incident types and personnel, and #96 for incidents, which is a bit more complicated (because we edit those).

@wsanchez
Copy link
Member Author

wsanchez commented Aug 3, 2016

OK I've adopted a library called lscache, which is has a memcache-like interface and seems appropriate at least for caching personnel and incident types.

It has the nice property that I don't have to write any cache expiration time logic myself.

I tested it with personnel and seems to work well enough to close #95.

I'm not sure I can use it for incidents if I also want to be able to subscribe to change events, though.

@wsanchez
Copy link
Member Author

wsanchez commented Aug 3, 2016

Back to this bug, the code to change is here, which is to say, we need to specify a context there.

@wsanchez
Copy link
Member Author

wsanchez commented Aug 3, 2016

046ad8a

@wsanchez wsanchez closed this as completed Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants