Fail operations when adapter is in destroy process #331
Conversation
xiazhvera
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
==========================================
- Coverage 82.41% 82.34% -0.07%
==========================================
Files 20 20
Lines 8722 8759 +37
==========================================
+ Hits 7188 7213 +25
- Misses 1534 1546 +12
☔ View full report in Codecov by Sentry. |
| * Set to true if the adapter is in destroying process (internal_ref is 0). The adapter would prevent any | ||
| * operations if `is_destroying` set to true | ||
| */ | ||
| bool is_destroying; |
There was a problem hiding this comment.
Judgement call: Instead of creating a new member here, we can use adapter_state to track the destroy processing. When we start the destroy process (internal ref -> 0), the adapter would not receive any lifecycle events anymore.
| @@ -327,6 +344,10 @@ static int s_aws_mqtt_client_connection_5_connect( | |||
| const struct aws_mqtt_connection_options *connection_options) { | |||
|
|
|||
| struct aws_mqtt_client_connection_5_impl *adapter = impl; | |||
| int result = s_check_adapter_destroy_status(adapter); | |||
There was a problem hiding this comment.
This check is missing for s_aws_mqtt_5_resubscribe_existing_topics, s_aws_mqtt_client_connection_5_acquire and s_aws_mqtt_client_connection_5_release. I think that's all, but I could miss something
There was a problem hiding this comment.
Updated s_aws_mqtt_5_resubscribe_existing_topics.
Debatable: I feel we should directly fail the acquire/release if the adapter is in destroying process... the ASSERT instead of a validation check might be a better option here.
There was a problem hiding this comment.
Yeah, this situation (trying to do something meaningful with connection in callback while it's being destroyed in another thread) should be pretty rare and most probably will indicate that something's wrong with user code. So, ASSERT could actually help to find a possible issue in the code.
|
Debatable: I feel slightly uneasy because of passing a "half-dead" |
|
Talked to Bret about the issue. The caller should be responsible to track the As Igor mentioned, passing a "half-dead" connection object into callbacks is hard to handle. However, if we pass a null here, it has a risk to crash the user. (If the caller forget to handle NULL here). We should make a rule to avoid expose the object in callbacks in the future implementation. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.