Don't call SSL_write() with num=0

[yzc-yzc]

valkey/src/networking.c

Lines 2279 to 2293 in aefed3d

	while (nwritten >= o->used) {
	next = listNextNode(curr);
	if (!next) break; /* End of list */
	nwritten -= o->used;
	o->refcount--;
	curr = next;
	o = listNodeValue(curr);
	o->refcount++;
	}
	serverAssert(nwritten <= o->used);
	c->repl_data->ref_repl_buf_node = curr;
	c->repl_data->ref_block_pos = nwritten;

From above code, we can see that c->repl_data->ref_block_pos could be equal to o->used.
When o->used == o->size, we may call SSL_write() with num=0 which does not comply with the openSSL specification.
(ref: https://docs.openssl.org/master/man3/SSL_write/#warnings)

What's worse is that it's still the case after the reconnection. See

valkey/src/replication.c

Lines 756 to 769 in aefed3d

	while (node != NULL) {
	replBufBlock *o = listNodeValue(node);
	if (o->repl_offset + (long long)o->used >= offset) break;
	node = listNextNode(node);
	}
	serverAssert(node != NULL);
	/* Install a writer handler first.*/
	prepareClientToWrite(c);
	/* Setting output buffer of the replica. */
	replBufBlock *o = listNodeValue(node);
	o->refcount++;
	c->repl_data->ref_repl_buf_node = node;
	c->repl_data->ref_block_pos = offset - o->repl_offset;

. So in this case the replica will keep reconnecting again and again until it doesn't meet the requirements for partial synchronization.

Resolves #2119

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.00%. Comparing base (aefed3d) to head (b24b55f).
⚠️ Report is 2 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2490      +/-   ##
============================================
- Coverage     72.05%   72.00%   -0.05%     
============================================
  Files           126      126              
  Lines         70446    70448       +2     
============================================
- Hits          50760    50727      -33     
- Misses        19686    19721      +35     

Files with missing lines	Coverage Δ
src/networking.c	87.92% <100.00%> (-0.10%)	⬇️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zuiderkwast]

Great to see progress on this!

So there can exist reply blocks (list nodes) with zero bytes? How/why do we create them?

[yzc-yzc]

So there can exist reply blocks (list nodes) with zero bytes? How/why do we create them?

e.g. The last node of the replication buffer list has used==size at some point, and we send all the data successfully. Then in function postWriteToReplica the replica client buffer will pointer to the last node, and the block_pos will be exactly the size. Then the next time we want to send data, it will be a multi block case. And we triggered SSL_write(_, _, 0).

[zuiderkwast]

Yeah, makes sense!

[zuiderkwast]

Looking at git blame, it seems that this bug was introduced in

• Offload replication writes to IO threads #1485

which was released in 8.1, so it means we need to backport this fix to 8.1 only. 8.0 and before are not affected AFAICT.