The way to finish a thread in volley.

[xyhuangjinfu]

In volley, to finish a thread such as CacheDiapatcher and NetworkDispatcher, are inturrupt not enough? Whether need the mQuit variable necessarily. If write like this, hava any problems? those thread can be interrupted in other scenes? public void quit() { interrupt(); }

while (!isInterrupted()) { ................... try { request = mQueue.take(); } catch (InterruptedException e) { return; } }

[xesam]

你为啥想要单独停止一个volley线程？？

[jpd236]

I agree it seems strange that we use a separate mQuit flag rather than just relying on the interrupt status. I don't see why you'd want to interrupt the threads but continue to have them pull ensuing requests off the queue.

So I could get behind a change which does what you describe and uses the interrupt status directly rather than relying on a separate mQuit flag.

[joebowbeer]

I agree that this looks fishy, but is there really a problem in practice? It would be great to have unit tests before making subtle changes.

Concerning the proper way to improve this, consider:

• If quit() is called before the thread is started, that is before the thread is alive, then a subsequent call to isInterrupted() may return false (according to spec).

• It is possible for the interrupt flag to be cleared by some other code executing on this thread, e.g., if an InterruptedException is caught or interrupted() is called without reasserting the interrupt flag.

These are reasons to maintain a dedicated cancellation flag and to check this flag inside the loop:

while (!mQuit) { ... }

Speaking of catching InterruptedException without reasserting the interrupt flag, note that standard practice is to reassert the interrupt flag before continuing:

} catch (InterruptedException e) {
  interrupt(); // restore interrupt flag
  return;
}

Arguably, this may not be useful here, as a thread stops running after returning from run()... OTOH, these are public Thread subclasses so without further analysis one can imagine they may themselves be subclassed or executed in surprising ways.

[jpd236]

If quit() is called before the thread is started, that is before the thread is alive, then a subsequent call to isInterrupted() may return false (according to spec).

Personally this feels like a minor concern because in practice, RequestQueue always starts threads immediately after creating them. You'd have to be doing something very custom with your queue or somehow manually be using CacheDispatcher/NetworkDispatcher outside the scope of a RequestQueue for this to be a potential issue, right?

It is possible for the interrupt flag to be cleared by some other code executing on this thread, e.g., if an InterruptedException is caught or interrupted() is called without reasserting the interrupt flag.

These are bugs that should be fixed (so long as they are happening in NetworkDispatcher/CacheDispatcher). This is a common mistake but usually a bug nonetheless, and certainly we should audit those classes for situations where this happens before moving to relying on the interrupted flag entirely.

[joebowbeer]

My main concern is the fragility of the Thread.interrupt flag. Given that this is a public Thread class, I think a dedicated private flag is a good idea. It clearly indicates that quit() was called and it will not be cleared according to the whim of some other code executing on this thread.

[xesam]

i have a question.why does volley use thread array but not Executor?

[jpd236]

Yeah, I can admit that getting interrupt right is hard (primary evidence being all the places in Volley that get it wrong), and that unfortunately with these classes being public / extensible we can't really control what code runs on them. Maybe it's best to just leave things the way they are here.

Re: thread array vs. Executor - what would an Executor allow us to do better? (This dates back to the beginning of Volley AFAIK, so I don't really know why it was done this way, but I also don't know that there's much reason to change it...)