Fix crash on repeated MockWebServer shutdown #5672
Conversation
The problem was the awaitIdle() call was scheduling executor jobs after the ExecutorService had been shut down. This fixes that and defends against other races.
swankjesse
force-pushed
the
jwilson.1230.await_idle
branch
from
5567730 to
edb5865
Compare
|
|
||
| @Test fun idleLatchAfterShutdown() { | ||
| queue.schedule("task") { | ||
| Thread.sleep(250) |
There was a problem hiding this comment.
nit: on it's own 250ms is fine. As part of a test suite 40 tests taking this approach add 10 seconds to running the tests and is still more flaky then using e.g. CountDownLatch here.
I've raised #5673 to fix such usages.
| if (existingTask is AwaitIdleTask) { | ||
| return existingTask.latch | ||
| } | ||
| for (futureTask in futureTasks) { |
There was a problem hiding this comment.
Not sure about practically, but do you need to check anything but the last task here? If it's non empty and has other tasks, seems by definition it's not idle.
There was a problem hiding this comment.
I think at the edges I’m conflating two concepts:
- idle: nothing running, nothing scheduled
- flush: complete all currently-enqueued tasks
In practice they’re the same effect for us, but I’ve implemented flush and labeled it idle. Because true idle is more work to implement for academic benefit I’m going to leave it as-is for now. Unsatisfying!
There was a problem hiding this comment.
One other wrench here . . . idle is distinct from shutdown. For example in our test suite we enforce idleness between tests, but share the same task runner between tests.
There was a problem hiding this comment.
Hopefully this is still a remaining cause of when our tests fail they tend to be flaky in batches, and we can fix a few remaining issues and have it rock solid.
Overall - trending ok https://circleci.com/build-insights/gh/square/okhttp/master
But big room for improvements once the base is solid.
The problem was the awaitIdle() call was scheduling executor jobs after
the ExecutorService had been shut down. This fixes that and defends
against other races.