Verify whether bufferevent_write is actually required for READONLY/CLIENT NO-TOUCH on discovered connections, or the original diagnosis was wrong

[fcostaoliveira]

Summary

send_conn_setup_commands() sends READONLY and CLIENT NO-TOUCH via bufferevent_write() instead of the plain protocol-level evbuffer_add() path every other setup-ladder command (AUTH, SELECT, HELLO, CLUSTER SLOTS) uses. The comment justifying this (originally written for READONLY in commit 0f53dc2, "replica READONLY flush + bootstrap-window hold", part of squashed PR #456; copied for CLIENT NO-TOUCH in PR #526) claims: for a connection attached via bufferevent_socket_new() + a later bufferevent_socket_connect() (the path every shard connection beyond the bootstrap seed uses, discovered via a live CLUSTER SLOTS reply), "the evbuffer notify callback does not re-arm EPOLLOUT on the first user-level send," so a plain evbuffer_add() write would sit in the output buffer forever.

Why this needs verification

Raised during PR #526 review (round 29): HELLO is sent via the exact same plain evbuffer_add_printf() path (redis_protocol::configure_protocol()), on the exact same kind of discovered connection, as the very first setup-ladder write -- and it demonstrably works. Empirically verified: attached redis-cli MONITOR to a non-seed shard in a local 3-node cluster and ran memtier with --cluster-mode (no special flags) against the bootstrap seed only. Real SET/GET traffic flowed successfully on the non-seed connection -- which requires m_hello to have reached setup_done, i.e. HELLO's plain-evbuffer-add write was sent and acknowledged, on the connection type the bufferevent_write comment says shouldn't flush a first plain write. (MONITOR itself doesn't show HELLO -- Redis filters it like AUTH -- so this is inferred from user traffic flowing, not observed directly.)

Also: bufferevent_enable(m_bev, EV_READ | EV_WRITE) already runs unconditionally at BEV_EVENT_CONNECTED, synchronously before send_conn_setup_commands() is ever called, and predates the original READONLY bug (it's present in ee757e7~1, before PR #456 merged) -- so the write-readiness watcher is armed before HELLO/READONLY/NO-TOUCH's first byte goes out either way.

The original bug-fix commit also fixed a second, unrelated hang in the same diff: fill_pipeline could spin forever generating no request when no replica was live yet under --read-preference != primary, starving the event loop of a chance to ever fire BEV_EVENT_CONNECTED for the in-progress replica connection at all. That's a plausible alternative explanation for the originally observed hang -- if the replica connection's BEV_EVENT_CONNECTED never fired, READONLY's bytes never got a chance to be sent regardless of which write path was used, and fixing the spin (unrelated to the write path) would have resolved the symptom on its own.

What's needed

Someone should actually isolate the two changes from PR #456 / commit 0f53dc2 (the hold_pipeline fill_pipeline-spin fix, and the READONLY bufferevent_write change) and determine which one was load-bearing for the original bug -- ideally by reverting just the bufferevent_write half against the fixed hold_pipeline and confirming READONLY still flushes correctly on a discovered replica connection.

If bufferevent_write turns out to be unnecessary, send_conn_setup_commands()'s READONLY and CLIENT NO-TOUCH sends could both be simplified back to the plain protocol-level path (and the misleading comments removed) -- but that's not urgent: bufferevent_write is safe either way (it doesn't change wire format or introduce risk), just possibly over-justified by an incorrect original diagnosis.

Discovered while working on

PR #526 (--client-no-touch) review, round 29.

[fcostaoliveira]

Related angle raised during PR #526 round 33: redis_protocol::write_command_readonly() (protocol.cpp) already exists as dead code -- it does a plain evbuffer_add(), but send_conn_setup_commands() bypasses it entirely with an inline static const char READONLY_CMD[] + bufferevent_write(), for the same reason CLIENT NO-TOUCH's NO_TOUCH_CMD now does. So this issue isn't just 'is bufferevent_write necessary' -- it's also 'why do AUTH/SELECT/HELLO/CLUSTER SLOTS construct their RESP bytes in protocol.cpp (the natural home) while READONLY and CLIENT NO-TOUCH construct theirs inline in shard_connection.cpp, with a same-named but unreachable method sitting unused in protocol.cpp'.

If the answer to the EPOLLOUT question turns out to be 'bufferevent_write genuinely isn't needed', the fix is straightforward: delete the two inline literals, wire READONLY and CLIENT NO-TOUCH through write_command_readonly()/a new write_command_client_no_touch() like every other setup command, done.

If the answer is 'bufferevent_write genuinely is needed for connections attached via bufferevent_socket_connect()', the cleaner fix probably isn't reachable by threading bufferevent access into abstract_protocol (protocol.h currently has no libevent/bufferevent dependency at all -- it only knows evbuffer via set_buffers(), which is a real architectural boundary, not an oversight). Whoever resolves the EPOLLOUT question should probably decide this one at the same time, since the right structure depends on the answer.