git-hulk commented on PR #3560: URL: https://github.com/apache/kvrocks/pull/3560#issuecomment-5009775605
> The async close is not guaranteed to invoke `Connection::OnWrite`, because blocking commands replace the bufferevent callbacks. > > For example, with: > > ``` > client-output-buffer-limit pubsub 1m 0 0 > ``` > > a RESP3 client can run: > > ``` > HELLO 3 > SUBSCRIBE ch > BLPOP never-exists 0 > ``` > > and another client can publish an 8 MiB message to `ch`. > > I reproduced this on this PR. `BlockingCommander::StartBlocking()` had replaced the write callback, so `bufferevent_trigger(..., EV_WRITE, ...)` invoked `BlockingCommander::OnWrite()` instead of `Connection::OnWrite()`. Since the list was still empty, it disabled `EV_WRITE` and returned without checking `kCloseAsync`. > > After 12 seconds the client was still present in `CLIENT LIST` with: > > ``` > cmd=blpop obuf=8388658 > ``` > > and `client_output_buffer_limit_disconnections` was already 1. The connection was only removed after the test client closed its socket. A non-blocked subscriber was closed normally in the control test. Yes, I know this issue > The async close is not guaranteed to invoke `Connection::OnWrite`, because blocking commands replace the bufferevent callbacks. > > For example, with: > > ``` > client-output-buffer-limit pubsub 1m 0 0 > ``` > > a RESP3 client can run: > > ``` > HELLO 3 > SUBSCRIBE ch > BLPOP never-exists 0 > ``` > > and another client can publish an 8 MiB message to `ch`. > > I reproduced this on this PR. `BlockingCommander::StartBlocking()` had replaced the write callback, so `bufferevent_trigger(..., EV_WRITE, ...)` invoked `BlockingCommander::OnWrite()` instead of `Connection::OnWrite()`. Since the list was still empty, it disabled `EV_WRITE` and returned without checking `kCloseAsync`. > > After 12 seconds the client was still present in `CLIENT LIST` with: > > ``` > cmd=blpop obuf=8388658 > ``` > > and `client_output_buffer_limit_disconnections` was already 1. The connection was only removed after the test client closed its socket. A non-blocked subscriber was closed normally in the control test. @jihuayu Yes, good catch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
