awslabs / aws-c-mqtt

C99 implementation of the MQTT 3.1.1 specification.
Apache License 2.0
93 stars 30 forks source link

Fail operations when adapter is in destroy process #331

Closed xiazhvera closed 11 months ago

xiazhvera commented 11 months ago

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.

codecov-commenter commented 11 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (c475ef1) 82.41% compared to head (ed5a700) 82.34%.

Additional details and impacted files ```diff @@ 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 ``` | [Files](https://app.codecov.io/gh/awslabs/aws-c-mqtt/pull/331?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=awslabs) | Coverage Δ | | |---|---|---| | [source/v5/mqtt5\_to\_mqtt3\_adapter.c](https://app.codecov.io/gh/awslabs/aws-c-mqtt/pull/331?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=awslabs#diff-c291cmNlL3Y1L21xdHQ1X3RvX21xdHQzX2FkYXB0ZXIuYw==) | `76.79% <83.78%> (+0.21%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/awslabs/aws-c-mqtt/pull/331/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=awslabs)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sfod commented 11 months ago

Debatable: I feel slightly uneasy because of passing a "half-dead" connection object into callbacks. But this approach seems safe with the current implementation, so I don't have strong opinion about it.

xiazhvera commented 11 months ago

Talked to Bret about the issue. The caller should be responsible to track the connection reference. If the caller have released the connection reference, the caller should never use connection even if the connection is from the callback.

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). Besides, it also breaks the contract that the user “should never use the connection if they have release it.” if they have to do a NULL check here.

We should make a rule to avoid expose the object in callbacks in the future implementation.