From b7e3f9d5a6b539d3a09e7886b579e3b51d32e543 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 23 Jan 2023 13:57:49 +0100 Subject: [PATCH 1/6] fix: try-case-of rather than try-of try-of catches only what happens within but not after --- .../src/emqx_resource_buffer_worker.erl | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/apps/emqx_resource/src/emqx_resource_buffer_worker.erl b/apps/emqx_resource/src/emqx_resource_buffer_worker.erl index 5460a8198..5b8524f39 100644 --- a/apps/emqx_resource/src/emqx_resource_buffer_worker.erl +++ b/apps/emqx_resource/src/emqx_resource_buffer_worker.erl @@ -266,11 +266,13 @@ code_change(_OldVsn, State, _Extra) -> %%============================================================================== -define(PICK(ID, KEY, PID, EXPR), - try gproc_pool:pick_worker(ID, KEY) of - PID when is_pid(PID) -> - EXPR; - _ -> - ?RESOURCE_ERROR(worker_not_created, "resource not created") + try + case gproc_pool:pick_worker(ID, KEY) of + PID when is_pid(PID) -> + EXPR; + _ -> + ?RESOURCE_ERROR(worker_not_created, "resource not created") + end catch error:badarg -> ?RESOURCE_ERROR(worker_not_created, "resource not created"); From a180bd9aa5197b93aaaa87c266bdc4c7a55d99d3 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 23 Jan 2023 13:56:33 +0100 Subject: [PATCH 2/6] fix: catch error, not exit --- apps/emqx_resource/src/emqx_resource_buffer_worker.erl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/emqx_resource/src/emqx_resource_buffer_worker.erl b/apps/emqx_resource/src/emqx_resource_buffer_worker.erl index 5b8524f39..0b0bf94aa 100644 --- a/apps/emqx_resource/src/emqx_resource_buffer_worker.erl +++ b/apps/emqx_resource/src/emqx_resource_buffer_worker.erl @@ -20,7 +20,6 @@ -module(emqx_resource_buffer_worker). -include("emqx_resource.hrl"). --include("emqx_resource_utils.hrl"). -include("emqx_resource_errors.hrl"). -include_lib("emqx/include/logger.hrl"). -include_lib("stdlib/include/ms_transform.hrl"). @@ -276,7 +275,7 @@ code_change(_OldVsn, State, _Extra) -> catch error:badarg -> ?RESOURCE_ERROR(worker_not_created, "resource not created"); - exit:{timeout, _} -> + error:timeout -> ?RESOURCE_ERROR(timeout, "call resource timeout") end ). From 8f3b1f87444d4b9d77d400e9f341a6cdc54dca1f Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 23 Jan 2023 16:37:24 +0100 Subject: [PATCH 3/6] chore: add changelog --- changes/v5.0.16/fix-9832.en.md | 1 + changes/v5.0.16/fix-9832.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/v5.0.16/fix-9832.en.md create mode 100644 changes/v5.0.16/fix-9832.zh.md diff --git a/changes/v5.0.16/fix-9832.en.md b/changes/v5.0.16/fix-9832.en.md new file mode 100644 index 000000000..84178b63c --- /dev/null +++ b/changes/v5.0.16/fix-9832.en.md @@ -0,0 +1 @@ +Improve error log when bridge in 'sync' mode timed out to get response. diff --git a/changes/v5.0.16/fix-9832.zh.md b/changes/v5.0.16/fix-9832.zh.md new file mode 100644 index 000000000..e7fd33b6b --- /dev/null +++ b/changes/v5.0.16/fix-9832.zh.md @@ -0,0 +1 @@ +优化桥接同步资源调用超时情况下的一个错误日志。 From 2d62de518802e26695c53cc54d1f39e385f5c50a Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Mon, 23 Jan 2023 16:42:23 +0100 Subject: [PATCH 4/6] test: fix expected result from timeout error --- .../test/emqx_resource_SUITE.erl | 27 ++++++++++--------- .../test/emqx_ee_bridge_influxdb_SUITE.erl | 8 +++--- .../test/emqx_ee_bridge_mysql_SUITE.erl | 9 ++++--- .../test/emqx_ee_bridge_pgsql_SUITE.erl | 12 ++++----- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/apps/emqx_resource/test/emqx_resource_SUITE.erl b/apps/emqx_resource/test/emqx_resource_SUITE.erl index 34a92a5a2..227b6fedc 100644 --- a/apps/emqx_resource/test/emqx_resource_SUITE.erl +++ b/apps/emqx_resource/test/emqx_resource_SUITE.erl @@ -19,8 +19,6 @@ -compile(export_all). -include_lib("eunit/include/eunit.hrl"). --include_lib("common_test/include/ct.hrl"). --include("emqx_resource.hrl"). -include_lib("stdlib/include/ms_transform.hrl"). -include_lib("snabbkaffe/include/snabbkaffe.hrl"). @@ -772,7 +770,10 @@ t_healthy_timeout(_) -> %% the ?TEST_RESOURCE always returns the `Mod:on_get_status/2` 300ms later. #{health_check_interval => 200} ), - ?assertError(timeout, emqx_resource:query(?ID, get_state, #{timeout => 1_000})), + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, + emqx_resource:query(?ID, get_state, #{timeout => 1_000}) + ), ?assertMatch({ok, _Group, #{status := disconnected}}, emqx_resource_manager:ets_lookup(?ID)), ok = emqx_resource:remove_local(?ID). @@ -1583,8 +1584,8 @@ do_t_expiration_before_sending(QueryMode) -> spawn_link(fun() -> case QueryMode of sync -> - ?assertError( - timeout, + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, emqx_resource:query(?ID, {inc_counter, 99}, #{timeout => TimeoutMS}) ); async -> @@ -1690,8 +1691,8 @@ do_t_expiration_before_sending_partial_batch(QueryMode) -> spawn_link(fun() -> case QueryMode of sync -> - ?assertError( - timeout, + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, emqx_resource:query(?ID, {inc_counter, 199}, #{timeout => TimeoutMS}) ); async -> @@ -2043,8 +2044,8 @@ do_t_expiration_retry(IsBatch) -> ResumeInterval * 2 ), spawn_link(fun() -> - ?assertError( - timeout, + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, emqx_resource:query( ?ID, {inc_counter, 1}, @@ -2127,8 +2128,8 @@ t_expiration_retry_batch_multiple_times(_Config) -> ), TimeoutMS = 100, spawn_link(fun() -> - ?assertError( - timeout, + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, emqx_resource:query( ?ID, {inc_counter, 1}, @@ -2137,8 +2138,8 @@ t_expiration_retry_batch_multiple_times(_Config) -> ) end), spawn_link(fun() -> - ?assertError( - timeout, + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, emqx_resource:query( ?ID, {inc_counter, 2}, diff --git a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl index e1899b1b2..2bac20bce 100644 --- a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl +++ b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl @@ -910,12 +910,10 @@ t_write_failure(Config) -> sync -> {_, {ok, _}} = ?wait_async_action( - try + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, send_message(Config, SentData) - catch - error:timeout -> - {error, timeout} - end, + ), #{?snk_kind := buffer_worker_flush_nack}, 1_000 ); diff --git a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_mysql_SUITE.erl b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_mysql_SUITE.erl index 57792b366..fec85c874 100644 --- a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_mysql_SUITE.erl +++ b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_mysql_SUITE.erl @@ -406,7 +406,10 @@ t_write_failure(Config) -> emqx_common_test_helpers:with_failure(down, ProxyName, ProxyHost, ProxyPort, fun() -> case QueryMode of sync -> - ?assertError(timeout, send_message(Config, SentData)); + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, + send_message(Config, SentData) + ); async -> send_message(Config, SentData) end @@ -439,8 +442,8 @@ t_write_timeout(Config) -> SentData = #{payload => Val, timestamp => 1668602148000}, Timeout = 1000, emqx_common_test_helpers:with_failure(timeout, ProxyName, ProxyHost, ProxyPort, fun() -> - ?assertError( - timeout, + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, query_resource(Config, {send_message, SentData, [], Timeout}) ) end), diff --git a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_pgsql_SUITE.erl b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_pgsql_SUITE.erl index 25752f685..6fbb9689f 100644 --- a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_pgsql_SUITE.erl +++ b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_pgsql_SUITE.erl @@ -426,12 +426,7 @@ t_write_failure(Config) -> ?wait_async_action( case QueryMode of sync -> - try - send_message(Config, SentData) - catch - error:timeout -> - {error, timeout} - end; + ?assertMatch({error, _}, send_message(Config, SentData)); async -> send_message(Config, SentData) end, @@ -467,7 +462,10 @@ t_write_timeout(Config) -> SentData = #{payload => Val, timestamp => 1668602148000}, Timeout = 1000, emqx_common_test_helpers:with_failure(timeout, ProxyName, ProxyHost, ProxyPort, fun() -> - ?assertError(timeout, query_resource(Config, {send_message, SentData, [], Timeout})) + ?assertMatch( + {error, {resource_error, #{reason := timeout}}}, + query_resource(Config, {send_message, SentData, [], Timeout}) + ) end), ok. From 7005b71ddfca5ee1c13a7902984c56b081f0faf7 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 24 Jan 2023 16:57:06 +0100 Subject: [PATCH 5/6] style: fix typo in comment --- apps/emqx_bridge/test/emqx_bridge_mqtt_SUITE.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/emqx_bridge/test/emqx_bridge_mqtt_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_mqtt_SUITE.erl index a99f06f20..cd5a17184 100644 --- a/apps/emqx_bridge/test/emqx_bridge_mqtt_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_mqtt_SUITE.erl @@ -899,7 +899,7 @@ t_mqtt_conn_bridge_egress_reconnect(_) -> ), Payload1 = <<"hello2">>, Payload2 = <<"hello3">>, - %% we need to to it in other processes because it'll block due to + %% We need to do it in other processes because it'll block due to %% the long timeout spawn(fun() -> emqx:publish(emqx_message:make(LocalTopic, Payload1)) end), spawn(fun() -> emqx:publish(emqx_message:make(LocalTopic, Payload2)) end), From 7d18128ba9dbae71b2af3fc9a55fdbc2b812ddca Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Wed, 25 Jan 2023 11:46:52 +0100 Subject: [PATCH 6/6] test: async write can return noproc --- lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl index 2bac20bce..cd7f848c2 100644 --- a/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl +++ b/lib-ee/emqx_ee_bridge/test/emqx_ee_bridge_influxdb_SUITE.erl @@ -945,7 +945,8 @@ t_write_failure(Config) -> {error, {recoverable_error, {closed, "The connection was lost."}}} =:= Result orelse {error, {error, closed}} =:= Result orelse - {error, {recoverable_error, econnrefused}} =:= Result, + {error, {recoverable_error, econnrefused}} =:= Result orelse + {error, {recoverable_error, noproc}} =:= Result, #{got => Result} ) end,