Redo TaskFaker's internal queue #8348
Conversation
Previously this used locks to stop threads from executing. Now it uses a custom 'yield' function to accomplish a similar purpose. The main benefit of the new approach is we're no longer subject to unspecified lock release order - we maintain our own queue and get true deterministic order of execution.
swankjesse
force-pushed
the
jwilson.0407.taskfaker
branch
from
b8dd85d
to
27cecd1
Compare
| * @param resumeEagerly true to prioritize the current task over other queued tasks. | ||
| * @param yieldUntilExhausted true to keep yielding until there's no other runnable tasks. | ||
| */ | ||
| private tailrec fun yieldUntil( |
| "plan 0 cancel", | ||
| "plan 0 TCP connect canceled", |
| @@ -78,7 +79,6 @@ class RealWebSocketTest { | |||
| @Test | |||
| fun close() { | |||
| client.webSocket!!.close(1000, "Hello!") | |||
| taskFaker.runTasks() | |||
There was a problem hiding this comment.
This was a tough change to review. There's several different concurrency mechanisms here that have to be coordinated just right. Even now I feel like I've only internalized about 2/3 of how it all works. Which (along with passing tests) is enough for me to feel good approving it, but I don't envy any non-Jesses who want to touch this code in the future.
By the way, you also could (should?) uncomment the ConnectionPoolTest.connectionPreWarming() as part of this PR.
| private var tasksRunningCount = 0 | ||
| private val tasksExecutor = | ||
| Executors.newCachedThreadPool( | ||
| object : ThreadFactory { |
There was a problem hiding this comment.
Nit: should we throw this into a little utility class somewhere? e.g.
class NamedThreadFactory(val prefix): ThreadFactory {
private var nextId = 1
override fun newThread(runnable: Runnable) = Thread(runnable, "$prefix-${nextId++}")
}
There was a problem hiding this comment.
Good idea. Coming up in follow-up.
| resumeEagerly: Boolean = false, | ||
| yieldUntilExhausted: Boolean = false, |
There was a problem hiding this comment.
It feels like these two parameters should be mutually exclusive; it doesn't make sense to use them both. If a caller does set both to true then I believe yieldUntilExhausted will have no effect.
Maybe replace these two params with a single enum parameter?
enum class YieldStrategy {
Eager, // Prioritizes this task
Lazy, // Prioritizes all other currently queued tasks
UntilExhausted, // Prioritizes all other queued tasks, including new tasks enqueued while waiting
}
(wordsmith as needed)
There was a problem hiding this comment.
Wordsmitthed! I turned your great comments into the member names.
private enum class ResumePriority {
/** Resumes as soon as the condition is satisfied. */
BeforeOtherTasks,
/** Resumes after the already-enqueued tasks. */
AfterEnqueuedTasks,
/** Resumes after all other tasks, including tasks enqueued while yielding. */
AfterOtherTasks,
}
| stall() | ||
| taskRunner.assertThreadDoesntHoldLock() | ||
| taskRunner.lock.withLock { | ||
| yieldUntil() |
There was a problem hiding this comment.
This method call is confusing at first glance until you go read all the default param values.
But I don't have any suggestions to make it better 🤷
1. The first test just needed to be uncommented now that #8348 is merged 2. A second test was added to exercise http2-specific functionality
1. The first test just needed to be uncommented now that #8348 is merged 2. A second test was added to exercise http2-specific functionality
1. The first test just needed to be uncommented now that #8348 is merged 2. A second test was added to exercise http2-specific functionality
Previously this used locks to stop threads from executing. Now it uses a custom 'yield' function to accomplish a similar purpose. The main benefit of the new approach is we're no longer subject to unspecified lock release order - we maintain our own queue and get true deterministic order of execution.