alaurenz / metrobike

0 stars 1 forks source link

Searching Dialog Not Cancelable #80

Closed CoolCapri closed 11 years ago

CoolCapri commented 11 years ago

Currently Searching Dialog is not cancelable, this is inconvenient when internet connection is lost and timeout for reporting this lost connection is long. I will improve it in Beta phase.

dutchscout commented 11 years ago

I think it will be hard to kill the search process; it is several levels of code deep when this is happening. That said, I set the internet retries (5x) and time-between-retries (500ms) manually, so we can make it fail as quickly as we want.

CoolCapri commented 11 years ago

OK. This seems much better when you can shorten the wait time. Thanks.

dutchscout commented 11 years ago

Actually, let's keep this issue open until we shorten the wait time... How long should it be?

CoolCapri commented 11 years ago

I pulled the latest snapshot and now it seems like it will report connection error after 5 seconds.

CoolCapri commented 11 years ago

And I think having 5 seconds is good. What does everyone think?

CoolCapri commented 11 years ago

Just found another issue with noncancelable feature. If the connection speed is low or the request is large, it would take a fair amount of time to respond to user and user cannot do anything but wait. So make it cancelable is still useful. It is easy on UI side, but I dont know about backend.

dutchscout commented 11 years ago

This is actually a bit complicated.

Basically, the UI is running a thread, which dives into the backend, runs through several algorithms (which each include blocking web requests), and finally exits when the results are complete.

If we wanted to allow a cancellation during the search, we have a few options:

1) Ignore the running thread. Basically, we could set a flag so the UI thread would quit immediately after the call to doRequest() returns.

2) Attempt to kill the search Thread object There are a couple of ways to try to kill a Java thread: stop() and interrupt(). (see here: http://stackoverflow.com/questions/5241822/is-it-good-way-to-stop-java-thread-forcefully )Unfortunately, stop() is deprecated and unsafe to use, and interrupt() is a bit complicated. (see http://docs.oracle.com/javase/6/docs/api/java/lang/Thread.html#interrupt() ). It looks like we could try it, but I'm not 100% sure what will happen.

3) Insert a flag to the URL querying function in the utility class to kill any pending request, making it look like the system lost Internet connectivity.

Of all of these options, I think I like the kill-the-Internet flag best, since our algorithm search would stop naturally if it realized there is no Internet access.

CoolCapri commented 11 years ago

Yeah, using flag seems more doable.

dutchscout commented 11 years ago

OK. I've created an issue for this in the backend: https://github.com/alaurenz/metrobike/issues/118

I believe the way you will need to use this is:

1) You currently have a separate thread in the UI running the request.

2) When a user presses cancel, the UI starts another thread. This thread: a) calls a function to disable the backend b) waits until the doRequest() call in the other UI thread returns (you will need to add a flag in the first UI thread so it doesn't return the cancelled results) c) calls a function to enable the backend Note: you shouldn't let the user start another request until the previous one has finished 'cancelling' or else the new one will be cancelled too.

As seen above, this cancelling thing is a bit tricky to use. If you don't understand the steps above, please let me know! All that the backend will provide are two (hopefully thread safe) methods called enableBackend() and disableBackend(). The UI will be responsible for waiting for backend calls to complete after calling disableBackend() and will be responsible for calling enableBackend() before making any new calls.

Please watch the other issue for when this feature is added to the backend (hopefully today).

CoolCapri commented 11 years ago

Sure. I will track of this.

dutchscout commented 11 years ago

Can we move the 'cancel search' feature to V1?

CoolCapri commented 11 years ago

Yes, I have moved it to V1 since this really takes a lot of time.

CoolCapri commented 11 years ago

I see. I am currently working on refracting search. I will add it to UI once I have finished.

CoolCapri commented 11 years ago

@dutchscout Since the cancel feature did not pass test on Jenkins. Should I add it right now or after you have fixed the problem? I am just concerning the code freeze time.

dutchscout commented 11 years ago

Add it now... the error in Jenkins is an 'incorrect travel mode error', which doesn't sound related to cancelling the search.

I may look into the travel mode error, though I'm working on filtering right now.

dutchscout commented 11 years ago

I think I found the source of the error locally. Testing locally again now...

mengwan commented 11 years ago

I removed the transit mod in the do request. Now I think the test case should fall into the mixed mode. But the problem is in mixed mode there are three alg running but it looks like they do not all get status connection_cancel...

mengwan commented 11 years ago

I think I got it, I will make a push