Bug #5720
closed
[Workbench] Search for pipeline templates breaks
Description
Files
Updated by Bryan Cosca over 9 years ago
I was pressing run a pipeline in project: qr1hi-j7d0g-l1emsbyyx6wyjdh and the template was from project: qr1hi-j7d0g-l1emsbyyx6wyjdh
Updated by
Updated by Radhika Chippada over 9 years ago
- Subject changed from Search for pipeline templates breaks to [Workbench] Search for pipeline templates breaks
- Category set to Workbench
Updated by Tom Clegg over 9 years ago
Is this just happening when the search term ends with a space, or is that a coincidence?
If that's not it, it would be good to collect some more evidence/clues to figure out what's happening:- Browser network activity
- Workbench logs
Updated by Radhika Chippada over 9 years ago
This issue is purely related to the ajax reloading. A few scenarios where the issue can be experienced:
- I think what happens is if there was an interruption while the page is loading, the error is displayed. You can reproduce it consistently using the steps below:
- Go to a project page. It starts loading.
- After you see the loading spinner, and while it is still showing*, hit enter or click on reload in address bar.
- You will see “Oops request failed. Retry”. The page does load fine if I just leave it alone.
- You can also see this issue using the following: Go to a pipeline_instance page. While it is loading and when you see the loading spinner, click on Home project link or select a project from the Projects drop down.
- Another scenario. Go to dashboard. When the dashboard is reloading (which happens once every 15 seconds), click on any link in the page such as a pipeline instance link. That is, click on a link while the red loading spinner is being displayed in the top-nav. You will see a "Reload tab" button with "An error occured" message. See https://arvados.org/attachments/download/564/Reload-tab-error.png
- One more scenario: While in dashboard and while the loading spinner is shown in top-nav, enter a uuid in search box and hit enter. You will see the reload tab button
- In case of Bryan's search, I was able to reproduce it with much difficulty and not quite reliably using this method. Select a template from search results (or a collection while looking at other objects in the search dialog). While it is loading in preview panel, enter / change text in search filter.
Updated by Radhika Chippada over 9 years ago
- Status changed from New to In Progress
- Target version changed from Bug Triage to 2015-04-29 sprint
- Assigned To set to Radhika Chippada
Updated by Radhika Chippada over 9 years ago
Implementation notes:
It appears that the check for new ajax request is failing. I added console.log and found that the comparison below is failing. By adding JSON.stringify, I was able to resolve this issue.
if ($container.attr('data-infinite-serial') != this.serial)
console log:
$container.attr('data-infinite-serial’) = "1429552990896"
this.serial = 1429552990896
Updated by Radhika Chippada over 9 years ago
- File Reload-tab-error.png Reload-tab-error.png added
Updated by Brett Smith over 9 years ago
Reviewing e5f039f
It seems like the basic issue here is that we're taking a non-string type (a Date) and putting it into an attribute, where it has to be stringified. From there, we have to remember to redo the stringification every time we want to do the comparison.
Is it possible to attach the original Date directly to the element by using .data() rather than .attr()? I don't see any reason why this needs to be an attribute per se, and it would let the comparisons work without any need for coercion.
Part of the reason I'm suggesting this is because there's another place later in the file where the same comparison happens, and that hasn't been fixed in this branch. This suggests to me that making the comparison easier to do is going to be more less error-prone in the long term than requiring this incantation every time.
Thanks.
Updated by Radhika Chippada over 9 years ago
- Target version changed from 2015-04-29 sprint to 2015-05-20 sprint
Updated by 
Updated by Tom Clegg over 9 years ago
+1 to note-9. $container.data('infinite-serial') can hold any js object... like a Date.
$('body').data('foobar', new Date())
[<body>…</body>]
d=$('body').data('foobar')
Thu May 07 2015 16:57:44 GMT-0400 (EDT)
d.getMilliseconds()
573
Updated by Radhika Chippada over 9 years ago
In IRC:
tom@curoverse.com
5:06 if (jqxhr.readyState 0 || jqxhr.status 0) {
message = "Cancelled."
5:06 in infinite_scroll.js
5:07 maybe we just need to check that in the tab-loading error handler as well, and do something similar?
Yes, this is indeed the problem. When readyState or status is 0, it could mean that the user has navigated away. I hence added logic to skip this error. Now I do not see the error in any of the scenarios I listed in note #5.
http://bartwullems.blogspot.com/2012/02/ajax-request-returns-status-0.html
Updated by Tom Clegg over 9 years ago
At acf8433
In tab_panes.js, the early "return" skips the $pane.removeClass('pane-loading').addClass('pane-loaded') state cleanup, which error handling normally does. We probably don't want to skip that.
In infinite_scroll.js, the early "return" skips the "remove div.spinner" code. It seems wrong to leave the spinner showing when we know the request has been cancelled.
But stepping back a bit, I'm a bit wary of "could mean the user has navigated away" as a reason to completely silence the failure feedback. What if you hit "forward", then "cancel", thereby cancelling the "forward" navigation in time to stay on the current page, but still interrupting the current page's progress?
While in dashboard and while the loading spinner is shown in top-nav, enter a uuid in search box and hit enter. You will see the reload tab button
This is another example where silently giving up could be worse than the current behavior. Of course, it would be much better to let the tab continue to load while the search box is opening -- but that sounds like its own bug, independent of the "what if tab loading is cancelled for some reason" issue.
If we used some sort of "un-cancelled" event to resume/retry loading in the relevant situations, it would make sense to go silent while the page is abandoned. But as it is, going silent just makes users more likely to end up with content areas that are inexplicably blank or have stopped refreshing.
(Is it documented somewhere that readyState==0 always means "cancelled by user"? If not, that's another reason not to just go silent: we could be unwittingly throwing away error feedback in non-user-initiated fail/cancel modes.)
Perhaps it would be better to say errhtml = 'Cancelled.' in both cases instead of going silent?
One other minor comment -- $foo.data('data-infinite-scroll') could be just $foo.data('infinite-scroll').
Updated by Tom Clegg over 9 years ago
(following up on IRC comments)
In tab_panes.js- if
$pane.attr('data-loaded-at') > 0then there is already valid content (i.e., this is just a refresh) it seems reasonable to leave the content alone, but we still need to fix up the pane-loading and pane-loaded classes. Ideally we'd also schedule a refresh in a few seconds, to avoid the case where "navigate away / cancel navigating away" causes us to skip the content update triggered by a websocket message that won't get triggered again (e.g., "pipeline complete"?) and let content stay stale indefinitely. - otherwise (i.e., when this is the initial tab load) it seems reasonable to show the "cancelled" message.
Updated by Radhika Chippada over 9 years ago
- Reverted the "skip" logic and using error message "Cancelled" when "jqxhr.readyState 0 || jqxhr.status 0". We would continue to see the unexpected error messages in the cases I listed above; however, if this is more desirable than skip, we now have it.
- Tom said "if $pane.attr('data-loaded-at') > 0 then there is already valid content ..." : I am not sure if I addressed this accurately. Please let me know if you have any further comments about this. Thanks.
Updated by Tom Clegg over 9 years ago
5720-ajax-loading-error looks good. I pushed (without actually trying) 6702cda to explain what I meant about "if data-loaded-at > 0 then...". Does this make sense?
Updated by Radhika Chippada over 9 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:f1827e2044aff826e63826880b296a59c4a17e2a.