-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move RealCall and RealConnection to loom safe locks #8290
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the best practices when synchronized
is only used to guard state, and never used for a blocking or slow call? From our concurrency rules, a bunch of these might be unnecessary?
### Locks
We have 3 different things that we synchronize on.
#### Http2Connection
This lock guards internal state of each connection. This lock is never held for blocking operations. That means that we acquire the lock, read or write a few fields and release the lock. No I/O and no application-layer callbacks.
#### Http2Stream
This lock guards the internal state of each stream. As above, it is never held for blocking operations. When we need to hold an application thread to block a read, we use wait/notify on this lock. This works because the lock is released while `wait()` is waiting.
#### Http2Writer
Socket writes are guarded by the Http2Writer. Only one stream can write at a time so that messages are not interleaved. Writes are either made by application-layer threads or the do-stuff-later pool.
...
#### Per-Connection Locks
Each connection has its own lock. The connections in the pool are all in a `ConcurrentLinkedQueue`. Due to data races, iterators of this queue may return removed connections. Callers must check the connection's `noNewExchanges` property before using connections from the pool.
The connection lock is never held while doing I/O (even closing a socket) to prevent contention.
A lock-per-connection is used to maximize concurrency.
(We definitely need the Http2Writer part of this)
Maybe to frame it more precisely:
Maybe we can find benchmarks on this. |
I'll create a tracking bug to benchmark. And review and rescope the locks. I'll land this as the tests show it's a real problem unless we change the locks. |
I don’t think we need to do any benchmarking work. For my own sake I just want an intuition on which locks to use where. |
This reverts commit a673f45.
@swankjesse Are we saying that the locks are not needed for RealCall, RealConnection, Dispatcher? That these should be reworked? |
…8290)" (square#8367) This reverts commit 689d388.
I think there’s two very different ways we use
I think we definitely want to change 2 to Loom-safe locks. I am unconvinced that there’s a benefit for changing 1. But there is a potential memory + performance cost. |
Main fix for #8284
replace synchronized with ReentrantLock.withLock, and wait/notify with lock.newCondition.signal/await.
Relies on test results from BasicLoomTest