From 66a4d6b2a493dd3a20cc299ab5fef3c14baad965 Mon Sep 17 00:00:00 2001 From: Chayim Date: Wed, 22 Mar 2023 18:03:50 +0200 Subject: [PATCH] AsyncIO Race Condition Fix (#2641) --- redis/asyncio/client.py | 12 +- redis/asyncio/cluster.py | 12 +- setup.py | 2 +- tests/asynctests | 285 +++++++++++++++++ tests/synctests | 421 ++++++++++++++++++++++++++ tests/test_asyncio/test_cluster.py | 17 ++ tests/test_asyncio/test_connection.py | 21 ++ 7 files changed, 764 insertions(+), 6 deletions(-) create mode 100644 tests/asynctests create mode 100644 tests/synctests diff --git a/redis/asyncio/client.py b/redis/asyncio/client.py index 3fc7fad83e..9e16ee08de 100644 --- a/redis/asyncio/client.py +++ b/redis/asyncio/client.py @@ -1385,10 +1385,16 @@ async def execute(self, raise_on_error: bool = True): conn = cast(Connection, conn) try: - return await conn.retry.call_with_retry( - lambda: execute(conn, stack, raise_on_error), - lambda error: self._disconnect_raise_reset(conn, error), + return await asyncio.shield( + conn.retry.call_with_retry( + lambda: execute(conn, stack, raise_on_error), + lambda error: self._disconnect_raise_reset(conn, error), + ) ) + except asyncio.CancelledError: + # not supposed to be possible, yet here we are + await conn.disconnect(nowait=True) + raise finally: await self.reset() diff --git a/redis/asyncio/cluster.py b/redis/asyncio/cluster.py index 5a2dffdd1d..569a0765f8 100644 --- a/redis/asyncio/cluster.py +++ b/redis/asyncio/cluster.py @@ -1002,10 +1002,18 @@ async def execute_command(self, *args: Any, **kwargs: Any) -> Any: await connection.send_packed_command(connection.pack_command(*args), False) # Read response + return await asyncio.shield( + self._parse_and_release(connection, args[0], **kwargs) + ) + + async def _parse_and_release(self, connection, *args, **kwargs): try: - return await self.parse_response(connection, args[0], **kwargs) + return await self.parse_response(connection, *args, **kwargs) + except asyncio.CancelledError: + # should not be possible + await connection.disconnect(nowait=True) + raise finally: - # Release connection self._free.append(connection) async def execute_pipeline(self, commands: List["PipelineCommand"]) -> bool: diff --git a/setup.py b/setup.py index a0710d3b73..3003c59420 100644 --- a/setup.py +++ b/setup.py @@ -8,7 +8,7 @@ long_description_content_type="text/markdown", keywords=["Redis", "key-value store", "database"], license="MIT", - version="4.5.2", + version="4.5.3", packages=find_packages( include=[ "redis", diff --git a/tests/asynctests b/tests/asynctests new file mode 100644 index 0000000000..4f0fea9223 --- /dev/null +++ b/tests/asynctests @@ -0,0 +1,285 @@ +test_response_callbacks +test_case_insensitive_command_names +test_command_on_invalid_key_type +test_acl_cat_no_category +test_acl_cat_with_category +test_acl_deluser +test_acl_genpass +test_acl_getuser_setuser +test_acl_list +test_acl_log +test_acl_setuser_categories_without_prefix_fails +test_acl_setuser_commands_without_prefix_fails +test_acl_setuser_add_passwords_and_nopass_fails +test_acl_users +test_acl_whoami +test_client_list +test_client_list_type +test_client_id +test_client_unblock +test_client_getname +test_client_setname +test_client_kill +test_client_kill_filter_invalid_params +test_client_kill_filter_by_id +test_client_kill_filter_by_addr +test_client_list_after_client_setname +test_client_pause +test_config_get +test_config_resetstat +test_config_set +test_dbsize +test_echo +test_info +test_lastsave +test_object +test_ping +test_slowlog_get +test_slowlog_get_limit +test_slowlog_length +test_time +test_never_decode_option +test_empty_response_option +test_append +test_bitcount +test_bitop_not_empty_string +test_bitop_not +test_bitop_not_in_place +test_bitop_single_string +test_bitop_string_operands +test_bitpos +test_bitpos_wrong_arguments +test_decr +test_decrby +test_delete +test_delete_with_multiple_keys +test_delitem +test_unlink +test_unlink_with_multiple_keys +test_dump_and_restore +test_dump_and_restore_and_replace +test_dump_and_restore_absttl +test_exists +test_exists_contains +test_expire +test_expireat_datetime +test_expireat_no_key +test_expireat_unixtime +test_get_and_set +test_get_set_bit +test_getrange +test_getset +test_incr +test_incrby +test_incrbyfloat +test_keys +test_mget +test_mset +test_msetnx +test_pexpire +test_pexpireat_datetime +test_pexpireat_no_key +test_pexpireat_unixtime +test_psetex +test_psetex_timedelta +test_pttl +test_pttl_no_key +test_randomkey +test_rename +test_renamenx +test_set_nx +test_set_xx +test_set_px +test_set_px_timedelta +test_set_ex +test_set_ex_timedelta +test_set_multipleoptions +test_set_keepttl +test_setex +test_setnx +test_setrange +test_strlen +test_substr +test_ttl +test_ttl_nokey +test_type +test_blpop +test_brpop +test_brpoplpush +test_brpoplpush_empty_string +test_lindex +test_linsert +test_llen +test_lpop +test_lpush +test_lpushx +test_lrange +test_lrem +test_lset +test_ltrim +test_rpop +test_rpoplpush +test_rpush +test_lpos +test_rpushx +test_scan +test_scan_type +test_scan_iter +test_sscan +test_sscan_iter +test_hscan +test_hscan_iter +test_zscan +test_zscan_iter +test_sadd +test_scard +test_sdiff +test_sdiffstore +test_sinter +test_sinterstore +test_sismember +test_smembers +test_smove +test_spop +test_spop_multi_value +test_srandmember +test_srandmember_multi_value +test_srem +test_sunion +test_sunionstore +test_zadd +test_zadd_nx +test_zadd_xx +test_zadd_ch +test_zadd_incr +test_zadd_incr_with_xx +test_zcard +test_zcount +test_zincrby +test_zlexcount +test_zinterstore_sum +test_zinterstore_max +test_zinterstore_min +test_zinterstore_with_weight +test_zpopmax +test_zpopmin +test_bzpopmax +test_bzpopmin +test_zrange +test_zrangebylex +test_zrevrangebylex +test_zrangebyscore +test_zrank +test_zrem +test_zrem_multiple_keys +test_zremrangebylex +test_zremrangebyrank +test_zremrangebyscore +test_zrevrange +test_zrevrangebyscore +test_zrevrank +test_zscore +test_zunionstore_sum +test_zunionstore_max +test_zunionstore_min +test_zunionstore_with_weight +test_pfadd +test_pfcount +test_pfmerge +test_hget_and_hset +test_hset_with_multi_key_values +test_hset_without_data +test_hdel +test_hexists +test_hgetall +test_hincrby +test_hincrbyfloat +test_hkeys +test_hlen +test_hmget +test_hmset +test_hsetnx +test_hvals +test_hstrlen +test_sort_basic +test_sort_limited +test_sort_by +test_sort_get +test_sort_get_multi +test_sort_get_groups_two +test_sort_groups_string_get +test_sort_groups_just_one_get +test_sort_groups_no_get +test_sort_groups_three_gets +test_sort_desc +test_sort_alpha +test_sort_store +test_sort_all_options +test_sort_issue_924 +test_cluster_addslots +test_cluster_count_failure_reports +test_cluster_countkeysinslot +test_cluster_delslots +test_cluster_failover +test_cluster_forget +test_cluster_info +test_cluster_keyslot +test_cluster_meet +test_cluster_nodes +test_cluster_replicate +test_cluster_reset +test_cluster_saveconfig +test_cluster_setslot +test_cluster_slaves +test_readwrite +test_readonly_invalid_cluster_state +test_readonly +test_geoadd +test_geoadd_invalid_params +test_geodist +test_geodist_units +test_geodist_missing_one_member +test_geodist_invalid_units +test_geohash +test_geopos +test_geopos_no_value +test_old_geopos_no_value +test_georadius +test_georadius_no_values +test_georadius_units +test_georadius_with +test_georadius_count +test_georadius_sort +test_georadius_store +test_georadius_store_dist +test_georadiusmember +test_xack +test_xadd +test_xclaim +test_xclaim_trimmed +test_xdel +test_xgroup_create +test_xgroup_create_mkstream +test_xgroup_delconsumer +test_xgroup_destroy +test_xgroup_setid +test_xinfo_consumers +test_xinfo_stream +test_xlen +test_xpending +test_xpending_range +test_xrange +test_xread +test_xreadgroup +test_xrevrange +test_xtrim +test_bitfield_operations +test_bitfield_ro +test_memory_stats +test_memory_usage +test_module_list +test_binary_get_set +test_binary_lists +test_22_info +test_large_responses +test_floating_point_encoding diff --git a/tests/synctests b/tests/synctests new file mode 100644 index 0000000000..b0de2d1ba9 --- /dev/null +++ b/tests/synctests @@ -0,0 +1,421 @@ +test_response_callbacks +test_case_insensitive_command_names +test_auth +test_command_on_invalid_key_type +test_acl_cat_no_category +test_acl_cat_with_category +test_acl_dryrun +test_acl_deluser +test_acl_genpass +test_acl_getuser_setuser +test_acl_help +test_acl_list +test_acl_log +test_acl_setuser_categories_without_prefix_fails +test_acl_setuser_commands_without_prefix_fails +test_acl_setuser_add_passwords_and_nopass_fails +test_acl_users +test_acl_whoami +test_client_list +test_client_info +test_client_list_types_not_replica +test_client_list_replica +test_client_list_client_id +test_client_id +test_client_trackinginfo +test_client_tracking +test_client_unblock +test_client_getname +test_client_setname +test_client_kill +test_client_kill_filter_invalid_params +test_client_kill_filter_by_id +test_client_kill_filter_by_addr +test_client_list_after_client_setname +test_client_kill_filter_by_laddr +test_client_kill_filter_by_user +test_client_pause +test_client_pause_all +test_client_unpause +test_client_no_evict +test_client_reply +test_client_getredir +test_hello_notI_implemented +test_config_get +test_config_get_multi_params +test_config_resetstat +test_config_set +test_config_set_multi_params +test_failover +test_dbsize +test_echo +test_info +test_info_multi_sections +test_lastsave +test_lolwut +test_reset +test_object +test_ping +test_quit +test_role +test_select +test_slowlog_get +test_slowlog_get_limit +test_slowlog_length +test_time +test_bgsave +test_never_decode_option +test_empty_response_option +test_append +test_bitcount +test_bitcount_mode +test_bitop_not_empty_string +test_bitop_not +test_bitop_not_in_place +test_bitop_single_string +test_bitop_string_operands +test_bitpos +test_bitpos_wrong_arguments +test_bitpos_mode +test_copy +test_copy_and_replace +test_copy_to_another_database +test_decr +test_decrby +test_delete +test_delete_with_multiple_keys +test_delitem +test_unlink +test_unlink_with_multiple_keys +test_lcs +test_dump_and_restore +test_dump_and_restore_and_replace +test_dump_and_restore_absttl +test_exists +test_exists_contains +test_expire +test_expire_option_nx +test_expire_option_xx +test_expire_option_gt +test_expire_option_lt +test_expireat_datetime +test_expireat_no_key +test_expireat_unixtime +test_expiretime +test_expireat_option_nx +test_expireat_option_xx +test_expireat_option_gt +test_expireat_option_lt +test_get_and_set +test_getdel +test_getex +test_getitem_and_setitem +test_getitem_raises_keyerror_for_missing_key +test_getitem_does_not_raise_keyerror_for_empty_string +test_get_set_bit +test_getrange +test_getset +test_incr +test_incrby +test_incrbyfloat +test_keys +test_mget +test_lmove +test_blmove +test_mset +test_msetnx +test_pexpire +test_pexpire_option_nx +test_pexpire_option_xx +test_pexpire_option_gt +test_pexpire_option_lt +test_pexpireat_datetime +test_pexpireat_no_key +test_pexpireat_unixtime +test_pexpireat_option_nx +test_pexpireat_option_xx +test_pexpireat_option_gt +test_pexpireat_option_lt +test_pexpiretime +test_psetex +test_psetex_timedelta +test_pttl +test_pttl_no_key +test_hrandfield +test_randomkey +test_rename +test_renamenx +test_set_nx +test_set_xx +test_set_px +test_set_px_timedelta +test_set_ex +test_set_ex_str +test_set_ex_timedelta +test_set_exat_timedelta +test_set_pxat_timedelta +test_set_multipleoptions +test_set_keepttl +test_set_get +test_setex +test_setnx +test_setrange +test_stralgo_lcs +test_stralgo_negative +test_strlen +test_substr +test_ttl +test_ttl_nokey +test_type +test_blpop +test_brpop +test_brpoplpush +test_brpoplpush_empty_string +test_blmpop +test_lmpop +test_lindex +test_linsert +test_llen +test_lpop +test_lpop_count +test_lpush +test_lpushx +test_lpushx_with_list +test_lrange +test_lrem +test_lset +test_ltrim +test_rpop +test_rpop_count +test_rpoplpush +test_rpush +test_lpos +test_rpushx +test_scan +test_scan_type +test_scan_iter +test_sscan +test_sscan_iter +test_hscan +test_hscan_iter +test_zscan +test_zscan_iter +test_sadd +test_scard +test_sdiff +test_sdiffstore +test_sinter +test_sintercard +test_sinterstore +test_sismember +test_smembers +test_smismember +test_smove +test_spop +test_spop_multi_value +test_srandmember +test_srandmember_multi_value +test_srem +test_sunion +test_sunionstore +test_debug_segfault +test_script_debug +test_zadd +test_zadd_nx +test_zadd_xx +test_zadd_ch +test_zadd_incr +test_zadd_incr_with_xx +test_zadd_gt_lt +test_zcard +test_zcount +test_zdiff +test_zdiffstore +test_zincrby +test_zlexcount +test_zinter +test_zintercard +test_zinterstore_sum +test_zinterstore_max +test_zinterstore_min +test_zinterstore_with_weight +test_zpopmax +test_zpopmin +test_zrandemember +test_bzpopmax +test_bzpopmin +test_zmpop +test_bzmpop +test_zrange +test_zrange_errors +test_zrange_params +test_zrangestore +test_zrangebylex +test_zrevrangebylex +test_zrangebyscore +test_zrank +test_zrem +test_zrem_multiple_keys +test_zremrangebylex +test_zremrangebyrank +test_zremrangebyscore +test_zrevrange +test_zrevrangebyscore +test_zrevrank +test_zscore +test_zunion +test_zunionstore_sum +test_zunionstore_max +test_zunionstore_min +test_zunionstore_with_weight +test_zmscore +test_pfadd +test_pfcount +test_pfmerge +test_hget_and_hset +test_hset_with_multi_key_values +test_hset_with_key_values_passed_as_list +test_hset_without_data +test_hdel +test_hexists +test_hgetall +test_hincrby +test_hincrbyfloat +test_hkeys +test_hlen +test_hmget +test_hmset +test_hsetnx +test_hvals +test_hstrlen +test_sort_basic +test_sort_limited +test_sort_by +test_sort_get +test_sort_get_multi +test_sort_get_groups_two +test_sort_groups_string_get +test_sort_groups_just_one_get +test_sort_groups_no_get +test_sort_groups_three_gets +test_sort_desc +test_sort_alpha +test_sort_store +test_sort_all_options +test_sort_ro +test_sort_issue_924 +test_cluster_addslots +test_cluster_count_failure_reports +test_cluster_countkeysinslot +test_cluster_delslots +test_cluster_failover +test_cluster_forget +test_cluster_info +test_cluster_keyslot +test_cluster_meet +test_cluster_nodes +test_cluster_replicate +test_cluster_reset +test_cluster_saveconfig +test_cluster_setslot +test_cluster_slaves +test_readwrite +test_readonly_invalid_cluster_state +test_readonly +test_geoadd +test_geoadd_nx +test_geoadd_xx +test_geoadd_ch +test_geoadd_invalid_params +test_geodist +test_geodist_units +test_geodist_missing_one_member +test_geodist_invalid_units +test_geohash +test_geopos +test_geopos_no_value +test_old_geopos_no_value +test_geosearch +test_geosearch_member +test_geosearch_sort +test_geosearch_with +test_geosearch_negative +test_geosearchstore +test_geosearchstore_dist +test_georadius +test_georadius_no_values +test_georadius_units +test_georadius_with +test_georadius_count +test_georadius_sort +test_georadius_store +test_georadius_store_dist +test_georadiusmember +test_georadiusmember_count +test_xack +test_xadd +test_xadd_nomkstream +test_xadd_minlen_and_limit +test_xadd_explicit_ms +test_xautoclaim +test_xautoclaim_negative +test_xclaim +test_xclaim_trimmed +test_xdel +test_xgroup_create +test_xgroup_create_mkstream +test_xgroup_create_entriesread +test_xgroup_delconsumer +test_xgroup_createconsumer +test_xgroup_destroy +test_xgroup_setid +test_xinfo_consumers +test_xinfo_stream +test_xinfo_stream_full +test_xlen +test_xpending +test_xpending_range +test_xpending_range_idle +test_xpending_range_negative +test_xrange +test_xread +test_xreadgroup +test_xrevrange +test_xtrim +test_xtrim_minlen_and_length_args +test_bitfield_operations +test +test_bitfield_ro +test_memory_help +test_memory_doctor +test_memory_malloc_stats +test_memory_stats +test_memory_usage +test_latency_histogram_not_implemented +test_latency_graph_not_implemented +test_latency_doctor_not_implemented +test_latency_history +test_latency_latest +test_latency_reset +test_module_list +test_command_count +test_command_docs +test_command_list +test_command_getkeys +test_command +test_command_getkeysandflags +test_module +test_module_loadex +test_restore +test_restore_idletime +test_restore_frequency +test_replicaof +test_shutdown +test_shutdown_with_params +test_sync +test_psync +test_binary_get_set +test_binary_lists +test_22_info +test_large_responses +test_floating_point_encoding diff --git a/tests/test_asyncio/test_cluster.py b/tests/test_asyncio/test_cluster.py index 13e5e26ae3..0857c056c2 100644 --- a/tests/test_asyncio/test_cluster.py +++ b/tests/test_asyncio/test_cluster.py @@ -340,6 +340,23 @@ async def test_from_url(self, request: FixtureRequest) -> None: rc = RedisCluster.from_url("rediss://localhost:16379") assert rc.connection_kwargs["connection_class"] is SSLConnection + async def test_asynckills(self, r) -> None: + + await r.set("foo", "foo") + await r.set("bar", "bar") + + t = asyncio.create_task(r.get("foo")) + await asyncio.sleep(1) + t.cancel() + try: + await t + except asyncio.CancelledError: + pytest.fail("connection is left open with unread response") + + assert await r.get("bar") == b"bar" + assert await r.ping() + assert await r.get("foo") == b"foo" + async def test_max_connections( self, create_redis: Callable[..., RedisCluster] ) -> None: diff --git a/tests/test_asyncio/test_connection.py b/tests/test_asyncio/test_connection.py index e2d77fc1c3..d3b6285cfb 100644 --- a/tests/test_asyncio/test_connection.py +++ b/tests/test_asyncio/test_connection.py @@ -44,6 +44,27 @@ async def test_invalid_response(create_redis): await r.connection.disconnect() +async def test_asynckills(): + + for b in [True, False]: + r = Redis(single_connection_client=b) + + await r.set("foo", "foo") + await r.set("bar", "bar") + + t = asyncio.create_task(r.get("foo")) + await asyncio.sleep(1) + t.cancel() + try: + await t + except asyncio.CancelledError: + pytest.fail("connection left open with unread response") + + assert await r.get("bar") == b"bar" + assert await r.ping() + assert await r.get("foo") == b"foo" + + @pytest.mark.onlynoncluster async def test_single_connection(): """Test that concurrent requests on a single client are synchronised."""