Potential Command Duplication in _send_command_parse_response Retry Mechanism

[ManelCoutinhoSensei]

Hi there,

I have a conceptual problem with how the _send_command_parse_response is being grouped:

redis-py/redis/client.py

Lines 590 to 595 in ea01a30

	def _send_command_parse_response(self, conn, command_name, *args, **options):
	"""
	Send a command and parse the response
	"""
	conn.send_command(*args, **options)
	return self.parse_response(conn, command_name, **options)

My issue is that this method is called inside a retry mechanism:

redis-py/redis/client.py

Lines 623 to 628 in ea01a30

	return conn.retry.call_with_retry(
	lambda: self._send_command_parse_response(
	conn, command_name, *args, **options
	),
	lambda error: self._disconnect_raise(conn, error),
	)

Which means that if the Redis container goes down after send_command and before parse_response, the entire _send_command_parse_response will be retried. This means that any command with side effects (such as XADD) could be executed twice instead of just once, leading to unintended duplication.

Would love to hear thoughts on potential fixes for this issue. Let me know if you need any additional details!

[vladvildanov]

Hi @ManelCoutinhoSensei ! You have a point here, it doesn't makes sense to retry write operation on read failure, moreover it may lead to unintended disconnects because we have a data in a socket buffer that we doesn't process. We would take this issue into a count

redis-py/redis/connection.py

Line 1491 in ea01a30

if connection.can_read() and self.cache is None:

[excitoon]

If you don't want this "This means that any command with side effects (such as XADD) could be executed twice instead of just once, leading to unintended duplication." you need to either disable retries and handle them yourself or move all of that into repeatable transaction or whatever, in general case this is unsolvable problem.

[ManelCoutinhoSensei]

@excitoon, your explanation suggests that the retry system's limitations force users to disable it and manage retries themselves. However, the solution proposed in PR 3559 demonstrates that the duplication issue can be resolved within the existing framework. While the PR may not be perfect, it shows that addressing this problem without abandoning the built-in retry mechanism is indeed feasible.
Furthermore, I don't know if it makes much sense for a library to have a functionality that ultimately doesn't work/is unreliable

[vladvildanov]

#3559 (comment)

[ManelCoutinhoSensei]

Follow up here