Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to multithread locking and tests. #369

Merged
merged 22 commits into from
Nov 22, 2023

Conversation

dgarske
Copy link
Contributor

@dgarske dgarske commented Nov 1, 2023

  • Refactor of locking to better detect and handle edge cases. Allow partial writes on all packet types.
  • Fix for issue processing wait for a publish with ack on a different packet type causing the write state to get reset.
  • Fix issue with publish write only incorrectly removing pending response. ZD 16769.
  • Fix for cancelling publish to also attempt to cancel any publish responses expected (Qos 1 and 2).
  • Fix for read of fixed portion (2 bytes) of header when only 1 is returned.
  • Remove extra test non-block in MqttClient_Publish_ReadPayload (already handled in MqttSocket_Read).
  • Improvements to the testing
  • Cleanup return code 0=success (use MQTT_CODE_SUCCESS)

@dgarske dgarske self-assigned this Nov 1, 2023
@dgarske dgarske changed the title Improvements to multithread example and tests. Improvements to multithread locking and tests. Nov 4, 2023
@dgarske dgarske force-pushed the thread_tests branch 4 times, most recently from 4bcb429 to 81931da Compare November 6, 2023 19:28
@dgarske dgarske requested a review from embhorn November 7, 2023 20:27
@dgarske dgarske assigned embhorn and unassigned dgarske Nov 7, 2023
@dgarske dgarske assigned dgarske and unassigned embhorn Nov 8, 2023
@dgarske dgarske assigned embhorn and dgarske and unassigned dgarske and embhorn Nov 16, 2023
@@ -496,9 +497,8 @@ WOLFMQTT_API int MqttClient_WaitMessage_ex(
MqttObject *msg,
int timeout_ms);

#if defined(WOLFMQTT_MULTITHREAD) || defined(WOLFMQTT_NONBLOCK)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would cancel ever be used outside of non-block or multithread?

Copy link
Member

@embhorn embhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other issues discussed in chat

@dgarske dgarske requested a review from embhorn November 16, 2023 15:59
@dgarske dgarske assigned embhorn and unassigned dgarske Nov 20, 2023
@dgarske
Copy link
Contributor Author

dgarske commented Nov 20, 2023

@embhorn let's gets this merged in. I'll followup later with any issues found.

- name: wolfmqtt configure without TLS
env:
WOLFMQTT_NO_EXTERNAL_BROKER_TESTS: 1
run: ./configure --disable-tls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better as --enable-all --disable-tls so that more code paths are enabled

@embhorn embhorn self-requested a review November 20, 2023 22:31
@dgarske dgarske self-assigned this Nov 21, 2023
…se. ZD 16769. Remove extra test non-block in `MqttClient_Publish_ReadPayload` (already handled in `MqttSocket_Read`). Cleanup return code 0=success.
…acket type causing the write state to get reset.
…lti-threaded. If not using non-blocking mode consider it "done".
… encounter this between two thread, just not on same thread).
…r multi-thread. Fix for `--disable-tls` and added tests.
… works correctly during testing. Customer is seeing a cancel for an ACK become strayed in the pending response list (investigating).
@dgarske dgarske removed their assignment Nov 22, 2023
@dgarske
Copy link
Contributor Author

dgarske commented Nov 22, 2023

@embhorn I've sorted out the test issues. This PR is ready to review / merge.

Copy link
Member

@embhorn embhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. Tested various configs including SN client with DTLS and multithread.

@embhorn embhorn merged commit 8ba1cea into wolfSSL:master Nov 22, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants