Andrey Yarovoy created HDDS-17494:
-------------------------------------
Summary: close evicted XceiverClients outside the
XceiverClientManager cache lock
Key: HDDS-17494
URL: https://issues.apache.org/jira/browse/HDDS-17494
Project: Apache Ozone
Issue Type: Bug
Components: Ozone Client
Reporter: Andrey Yarovoy
Assignee: Andrey Yarovoy
*Problem*
`XceiverClientManager` closes a datanode client while holding the `clientCache`
monitor. Every
`acquireClient`/`releaseClient` call in the JVM synchronizes on that monitor.
While one thread closes a client, all
other reads and writes therefore wait to acquire or release theirs.
A client is closed once it is evicted and its reference count is 0
(`XceiverClientSpi.cleanup()`). That check, and
the close, run in three places, all under the monitor:
- **Removal listener.** `onRemoval` → `synchronized (clientCache)` →
`setEvicted()` → `close()`. Guava calls the
listener synchronously, on the thread whose cache operation evicted the entry:
- `acquireClient` → `clientCache.get(key, loader)`, which can evict by size or
expiry. It runs inside
`acquireClient`'s own `synchronized (clientCache)`.
- `releaseClient(client, invalidateClient=true)` →
`clientCache.invalidate(key)`, inside `releaseClient`'s
`synchronized (clientCache)`.
- The periodic `clientCache.cleanUp()` added by HDDS-17491. Here the listener
takes the monitor itself.
- **Release.** `releaseClient` → `decrementReference()` → `close()`, when an
already evicted client is released.
- **Manager close.** `close()` → `invalidateAll()` / `cleanUp()`, through the
listener.
How long a close can take:
- `XceiverClientGrpc.close()` calls `shutdownNow()`, then `awaitTermination`
for up to 5 s per channel.
- `XceiverClientRatis.close()` closes its `RaftClient`. That closes the gRPC
client, and with data streaming also the
`NettyClientStreamRpc`, which first flushes outstanding stream requests.
HDDS-15849 showed what this costs. When `XceiverClientGrpc.close()` waited
about 100 ms for a graceful shutdown,
eviction-driven closes serialized every concurrent `acquireClient`. It was
fixed by making the gRPC close fast
(`shutdownNow()`), not by moving the close out of the lock. Any close that
still waits blocks everyone else for its
duration, for example a channel that doesn't terminate promptly, or a Ratis
client with outstanding stream requests.
*Proposed change*
Decide under the lock which clients to close, and close them after leaving it.
- Change `XceiverClientSpi.setEvicted()` and `decrementReference()` to return
whether the client must now be closed
(reference count 0 and evicted), without closing it.
- In `XceiverClientManager`, add clients that must be closed to a
`pendingClose` queue while holding the monitor.
- After its `synchronized` block, every entry point closes what has been
queued: `acquireClient`, `releaseClient`,
the periodic cleanup task and `close()`. This is needed because the removal
listener runs inside cache calls that
are themselves made under the monitor.
Why this is safe:
- The lock still makes the reference-count check atomic with `acquireClient`'s
get-and-increment.
- An evicted client with no references can't be acquired again, because it is
no longer in the cache, and
`acquireClient` only takes clients from the cache. So closing it a moment
later, outside the lock, can't race with
a new user.
- `close()` is already idempotent in both client implementations.
Effect: a slow close delays only the thread that triggered it, often the
cleanup thread from HDDS-17491. It no longer
delays every other reader and writer.
*Testing*
- Unit test, with a client whose `close()` blocks on a latch:
- while it is being closed, `acquireClient` and `releaseClient` on other
pipelines still complete;
- evict it through each path: size eviction in `acquireClient`,
`releaseClient(..., true)`, the periodic
cleanup, and `close()`.
- Existing behaviour is unchanged:
- a referenced client isn't closed until its last release;
- every evicted, released client is eventually closed;
- `XceiverClientManager.close()` closes all clients before returning.
- `TestXceiverClientManagerCloseContention` and
`TestXceiverClientManagerIdleEviction` still pass.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
