[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

[dontfeedthecode]

@ayurmedia Awesome work mate, very impressed. That fix works like a charm!

[jlukic]

I'll review this when i return to development next week.

[jaimehing]

any updates on this? I'm experiencing the same problem

[kmd1970]

I am also having this issue and had to switch to JQuery ui autocomplete for now. I really love semantic ui, but there are a few core components that still don't work as expected. Keep up the good work!

[ayurmedia]

seriously what's going on with this bug, it's now open for month and still in evaluating phase, even its confimed as a serious bug several times. this reduces credibility of the semantic-ui team a lot. there is a workaround and solution for the bug. this is really lazy. the autocomplete, search is an essential gui widget and should work, or people use other frameworks if semantic-ui does not work.

Assignee:
No one assigned

why ? nobody responsible for the bug ?

[kmd1970]

I am sure everyone is working very hard, relax, try some earl grey.

[cheerfulstoic]

FYI I'm seeing this as well! Getting on the chain for updates!

[Anomen]

Hi there, just FYI, I see the same bug in my project. @ayurmedia's fix works, but I understand it's not the ideal.

[revosys]

Hey @ayurmedia

i need your help on this...

http://stackoverflow.com/questions/33719852/sementic-ui-search-module-with-api-issue

Thanks

[ayurmedia]

@revosys , seems your stackoverflow question has nothing to do with the bug, and you just need to read the manual to use semantic-ui search in the normal way. if you make a jsfiddle using my sandbox demo as a base for example, i might be able to help. the code snippets on stackoverflow don't help me understand your problem. here we discuss about the bug, that the abort-method is implemented wrong and should be fixed by the developers of semantic ui, still not fixed after 3 month.

[revosys]

hey @ayurmedia
thankx for your reply... here the fiddle - https://jsfiddle.net/0bkhfpzj/

what i want to know that how replace the URL with localhost "sub/data.json?q={query}" so that it search in "title" of my json file... and if result found than it show the result

here is my json file...
{
"items": [
{
"title": "Name",
"name": "guru"
},
{
"title": "Delly",
"name": "prog"
}
]
}

My issue is when i use the above code, and search something it show all result from json file without searching the typed words...

Thanx

[woodlandhunter]

@jlukic typing itself can abort previous ajax calls. Here's a self contained node/express script to reproduce the issue easily:

https://gist.github.com/anonymous/ee5cfbbe436b860c8ecf

(Edit: new gist which disabled cache)

I don't know if there's way to discern xhr requests aborted by continuing to type, but perhaps a solution would be to only fire onAbort when the aborted xhr request is the most recent xhr request.

[woodlandhunter]

@ayurmedia FYI, you can join the Gitter chat in the future to have off-topic discussion

[ayurmedia]

@woodlandhunter did you check my test-case i discribe in the post from aug 26. it makes the error reproducible 100% of the time because the crucial ajax is slowed down. the bug is only visible if there is a new xhr when the last xhr did not finish yet. (eg. you have 2 xhr in parallel). most developers use fast server and dont understand the bug. i have a complex autosuggest on the serverside so i cached the bug quickly and then improved my backend to give faster responsetimes also. there is no doubt the bug is real.

one dirty way could be to make the xhr more sync, eg. if a second xhr is opended then kill all old xhrs so the abort cannot kill the newest xhr. because the newest xhr is the best one for the time.

[ayurmedia]

i reported the bug at v 2.0.8 , now in 2.1.8 its still not fixed.

[larsbo]

the idea of tracking all active xhr requests and kill them instantly if a new one starts sounds good 👍

[woodlandhunter]

@ayurmedia I don't think anyone disagrees that there is a bug. You can see from the gist I provided that I understand the bug.

@jlukic I can confirm part of what is stated in the original report and outline the steps that happen:

• module.request is assigned when module.send() is called for the first query (here)
• setTimeout to rejectWith() first query is scheduled (here)
• module.request is re-assigned for second query (here)
• setTimeout fires and calls rejectWith() on the new module.request (here)

Storing the current value of module.request at least prevents rejecting the current query's Deferred, but you can still see the error message appear before the next query completes (to make this more obvious, using my gist, let all search queries have a 5 second delay). This isn't desirable in my opinion.

There's a similar use of the module.request property here

I hope I was clear enough in my explanation.

[ayurmedia]

@woodlandhunter the bug is still in status "evaluation" so its not 100% confirmed the bug is real.

your observation of the timeouts and rejectWith is correct as i can see. as its connected to module and not the xhr event seems to be the problem. the workaround like @larsbo +1 is to remove old xhr requests when the new xhr request is opened, to prevent a late call on onAbort which will then interfere with the new xhr. if a new xhr is started in the module (other xhrs could run from other modules at the same time) then old xhrs should be stopped immediately so that the late abort from the first xhr cannot influence the latest xhr. the bug only appears if there are 2 xhrs running at the same time.

a) user types
b) xhr is started, and loads response from server. (server is calulating and waiting with response)
c) user types again
d) xhr1 should now be killed with all attached things. xhr2 is started with the new typed word
e) xhr2 loads from servr (and waits becasue server is slow)
f) xhr1 realizes abort and sends onAbort (or rejectWith as they are connected), this will cause the errorhandling. and xhr2 will be broken, as xhr1 triggered the onAbort.
=> here is the bug, now "Server Issue" is displayed.
g) xhr2 gets a good response, but does not display the correct result, becaue xhr1 killed the module.

by ignoring the onAbort xhr2 can display the correct (but late) response. and is not broken by xhr1

[jlukic]

I am able to consistently reproduce the issue with network throttling.

Will have something soon.

[jlukic]

@ayurmedia @woodlandhunter @larsbo

I've made two changes to fix this.

The first, in API, onAbort now returns before hitting onFailure, generally I think it makes sense onAbort should be a separate callback independent of failure. This also makes it easier to distinguish an abort event inside onFailure. Previously calling .api('abort') was triggering the search error handler.

The second half of the issue was that search was using .api('setting', settings), on query, a method for changing the settings of the running instance (without destroying it).

This would cause the settings for the running instance to be modified to new onError callbacks.

I've fixed the issue by making sure full API init occurs on each request.

[kmd1970]

@ALL Thanks for working on this!

[jlukic]

I've also verified using APISettings with dropdown should be fine.

[brunofunnie]

The example supplied by @ayurmedia works but it's very hard to find the diff to generate a patch, also his example is using a very old version of semantic.js (2.0.8).

I'm still having this bug, and I am using the 2.1.8 version, someone already released a pull request ? Or there is a patch for this ?

[ayurmedia]

@brunofunny the bug is fixed already, you need version 2.1.9 or 2.2 , or the version here from github. 2.1.8 is the last version with the bug.

[brunofunnie]

Which branch is being used @ayurmedia ? I"ve tried the "master" and it's (at least in the comment) at 2.1.7 version, and I cannot find the 2.1.9 nor 2.2 version to download anywhere. Do you have the link for it ?

[brunofunnie]

Just for the notes, the release 2.2 is being maintained at the branch "next", there was the only place I found it. The version 2.1.9 I couldn't find at all.