[Search] - Ajax abort not correct "there was an issue querying the server."

[ayurmedia]

Steps to Reproduce

1. use searchfield with .ui.search and remote api
2. type some searchwords
3. quickly change the searchwords to trigger an ajax abort
4. it starts a new api search
5. it shows server-error because of the bug

Expected
The Search should do this:

1. Enter word, start search via ajax (ok)
2. Enter another word, abort ajax (ok)
3. Start another request (ok)
4. show results of the new request (broken)

Result
The Search does not do this:

1. The search shows server error, but should show result
2. ajax abort does not work on instance but globally on module.request

Bug is somewhere module.request.rejectWith, which will break ajax requests after this
$api.search.query.event.fail...
if one returns in fail() then search works like expected.

Testcase
the bug can be reproduced on the semantic-ui page:
http://semantic-ui.com/modules/search.html

when you quickly type different words in the searchbox you can produce "abort". (unfortunately semantic-ui-com often responds with 502 Bad Gateway, but it should be abort.

the GET is Aborted, and another GET is successful.
the API Debug now should trigger the display of the results, but because of global vars or a race-condition the second Ajax request also shows API => "API request failed" which is the cause of the Bug.
the error-message of the first GET gets copied over to the second GET (ajax) request and then shows the server-error, which is wrong, because the second request was successful.

if the callback of $api.search.query.event.fail: function()... returns early then the result-list will be shown.

to turn on debug log in js-console on semantic-ui website enter following in the javascript-console.

$('.ui.search').search({'debug':1,'verbose':1,'cache':0});

[dontfeedthecode]

The problem is easier to reproduce with a remote data source, it doesn't even really need to be typed that quickly. I've got a search component hooked up to a remote API and this error shows up pretty much any time you make a spelling mistake.

[jlukic]

Abort is on purpose, this prevents an edge case where a previous request takes longer to load than the last request.

So for example:

With abort:

1. "S" - starts loading S results
2. "Se" - self clearing timeout prevents search until user finishes typing..
3. "Sea" - User stops typing, 'Sea' begins querying,
4. "Sea" results load quickly and change the results to results for "Sea"
5. "S" the stubborn query that hit just too many DB rows, finally loads and replaces the results with "S" results (showing incorrect search results).

Without abort

1. "S" - starts loading S results
2. "Se" - checks against self clearing timeout for typing
3. "Sea" - User stops typing, 'Sea' begins querying.
4. "S" query aborts, preventing callbacks from occurring.
5. "Sea" results load as normal

[ayurmedia]

@jlukic the abort is correct, but the semantic-ui code has a bug. i have explained in detail the bug and the solution. the problem is that there are 2 ajax requests which are isolated and correct. but the semantic ui status code is a singleton and mixes the 2 requests which is wrong and the bug. the status must be attached to the request and not the query object. if the ...fail() function is handled correctly, then the first ajax request is aborted. then the second ajax requests finishes and the result will be shown correctly. the problem is "API Request failed... status abort" is copied from request 1 to request 2 which is a bug. the API request of the second ajax request should be something like ok. its clearly a bug which can be reproduced. i use the semantic-ui code in a project and i had to fix this fail() function so that remote request are handled correctly. otherwise i get this "server error" which is no server error, but a bug in handling the abort or more precicely handling the status code inside the semantic-ui api which does not separate the requests correctly but stores a global variable for the query function which is wrong. it works most of the time, but if ajax.abort is happening then its wrong and a bug which must be fixed.

[ayurmedia]

@jlukic "5) "S" the stubborn query that hit just too many DB rows, finally loads and replaces the results with "S" results (showing incorrect search results)."

this never happens, because the abort handles this correctly.by typing "Sea" the "S"-Request is aborted and stops the loading of the result of this request and the result thrown away. the loading of this request is stopped and it does not matter what the server returns.

abort must be used, there is no question about it. but the semantic-ui api code must handle the ...fail() correctly.

[jlukic]

The labels on the above steps were swapped by accident. "With abort" should have been "without abort" and vice versa, i've gone ahead and edited the above message.

I'm not sure I've experienced the fail conditions you're referring to. I'm aware the docs API endpoints produce 502 bad gateway errors. (Anyone want to donate a server to host the fake API endpoints?), but otherwise I'm not aware of any issues.

Can you outline a specific set of steps to reproduce the issue?

[ayurmedia]

i cannot see swap in your comment. again, the abort() which is triggered by the xhr ajax request is a good thing, because if you go ahead and change "S" to "Sea" then the request with "S" needs to abort, as its not valid anymore. you are now only interested in the result of "Sea". somehow the abort() call ends up in the semantic-ui api and sets the "Api Reqeust failed: ... statusText = "abort" " which is the behavior that we want. but in the semantic.js code is a bug which causes the second request with "Sea" to break, because the request with "S" set the status of the API to "abort" by changing a global variable, which is read by the second request. but it should be status "ok" as the status of request2 is ok. this then leads to a chain-reaction and all following requests are then aborted, even if they are loading correctly. i repeat myself. two (or more) objects write to the same api variable and overwrite each in a incorrect way. in the screenshot i have expanded the "v API" in the console, all the values are coming from the last request, but only the line "API error" comes from a global variable, the output in the console then is a mixed object, but it should only contain data from the request. this means the status error needs to be moved from the global API.query.errors to API.query.request1.errors , API.query.request2.errors etc. so that the error of one request dont interfere with the results of other requests. the code here is quite complicated with many callbacks so i try to explain the main problem here in a simplified way. i might try to create a demo-page with a fix or workaround to compare the broken and the fixed behavior. there is a real bug here and it can be fixed. thats why i posted this issue. its easily reproducible, even if its a corner-case, but many users will hit this bug, as the nature of a suggest is, that you interact with the results by typing while its loading which means you cause a lot of xhr.aborts which is ok, but must be handeld correctly by the semantic ui API. here the api is simply a function which takes care of the query-function which then handles the xhr request. the main problem is the usage of a global variable where one would need one variable per request then the bug would be fixed.

if you modify the code in $api.search.query.event.fail: function(){ return false;} then the error-status is not set and the suggest behaves like you want. the bug is now 20+ days open and nobody wants to fix it, because they don't understand the problem and the mistakes in the semantic.js code.

also the bug is a bit tricky to reproduce, so i might try to write some code you can put into console to trigger the error like

1. send request with "s",
2. abort request
3. send request with "Sea"
4. wait .

expected result: popup shows response from "Sea" query
reality: it shows "server has problem" (which is a lie).

[dontfeedthecode]

@ayurmedia did you work out a temporary fix for it? I'm definitely experiencing the same issue as you.

[daniyalnawaz]

I'm also having the same issue. You can even test on the documentation. Usually happen when a previous request gets aborted.

[ayurmedia]

@dontfeedthecode yes i have a fix/workaround. i will fork the github project and add a fix and make a pull request. the solution is to test if the error-type is abort and then return false in the ...fail() function. because we can simply ignore the abort at some point as it simply does not show results, which is actually the behavior we want. then the following ajax request will work correctly and show results because the API-status is not broken by the false handling of abort. didn't see side-effects yet. my first try was to simply ...function fail() { return; } which works, but it will ignore real server errors, but we want to display if the server had some real error like timeout , 503 or malformed json-response . important while developing to get feedback when your json is wrong and not just ignore the warning like abort. actually abort is not an error, abort is a user-interaction and a feature.

[ayurmedia]

@jlukic i might host a api response server on my private website, which will simply echo the search-string and return a valid json response. additionally with a delay of 1second or more so one can test the abort case in a relaxed way. because to reproduce the bug you need to abort before the server returns a 200 response, otherwise its not an abort. so if the server waits with the response for a while its easier to produce abort. i probably found the bug because my server-search response was slow, so i hit the abort quite often. if the server is very quick then it often returns a valid json with status 200 and no abort. but the semantic-ui server is broken as it often returns "false gateway" which makes it hard to test features. is it possible to host dynamic code on github like node.js or php ?

[ayurmedia]

I have created a sandbox to test the bug, there is also one with a solution:
http://php-ayurmedia.rhcloud.com/semantic/

you can also use this echo-server to play with your own implementation.
this api-server returns your serach-query and delays the result with the for delay seconds.
you can change the delay to test different cases. with 1 second delay the bug is easy to reproduce.
simply type in the form and and add or remove characters in the form-field.
http://php-ayurmedia.rhcloud.com/semantic/semantic-search.php?delay=1&search=demo

The test-form has 3 cases to test the bug easily, open developer console and network/xhr to see what happens or console.
a) 1 Second Delay which will produce abort easily
b) 0 Second Delay which hides the abort bug most of the time
c) 1 Second Delay and Broken Response, which correctly triggers "Server Issue" Message.

Here is an example form with the currently (master branch) broken 2.0.8 version of semantic.js
http://php-ayurmedia.rhcloud.com/semantic/search.2.0.8.html

Here is a form with the patched version
http://php-ayurmedia.rhcloud.com/semantic/search.patched.html

i also have a patched version on a cloned git-hub project here:
i only changed the dist/semantic.js , somebody can change the source at the correct position, the code structure is too complicated for me. actually the fix is in api.js and not search.js as one might expect. which means api is broken, and not only search.

https://github.com/ayurmedia/Semantic-UI/blob/master/dist/semantic.js
i have sent a pull-request for this workaround on dist/semantic.js

or
https://github.com/ayurmedia/Semantic-UI
(semantic.min.js is not patched !)

The workaround is at line 18370 in current (2.0.8) dist/semantic.js in github (just commenting out the line)

        if(timeLeft > 0) {
                module.debug('Response completed early delaying state change by', timeLeft);
              }
              setTimeout(function() {
                if( module.is.abortedRequest(xhr) ) {
                  //module.request.rejectWith(context, [xhr, 'aborted', httpMessage]);
                }
                else {
                  module.request.rejectWith(context, [xhr, 'error', status, httpMessage]);
                }
              }, timeLeft);
            }
          },

this workaround does not change the fail() function, but uses the existing test on "module.is.abortedRequest" which should be more correct with less side-effects.

aborted is not a real error, so rejectWith() is not the correct way to do it.
the request is already aborted so nothing to do, simply stop or skip.
abort was triggered by the user by typing a different search-word and starting a new request which then stops (cancels) the old request. its not an error, but a expected behavior.

if its a real error then still the error-message will be shown. (in the else case)

edit: improved markdown for better syntaxhighlight