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

pal inconsistency among implementations #110

Open
yamt opened this issue Oct 12, 2020 · 3 comments
Open

pal inconsistency among implementations #110

yamt opened this issue Oct 12, 2020 · 3 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@yamt
Copy link
Contributor

yamt commented Oct 12, 2020

consider the underlying socket is non-blocking.

  1. the socket version of mqtt_pal_sendall seems to return
    MQTT_ERROR_SOCKET_ERROR on any errors, including EAGAIN.

    other implementations look similar.
    except the mbedtls version, which returns the bytes sent in case of EAGAIN-like conditions.

    IMO, the behavior of mbedtls version is better because
    __mqtt_send needs to know who many bytes has been sent successfully.
    (to update send_offset)

  2. the socket version of mqtt_pal_recvall ignores EAGAIN and keeps busy looping.
    the mbedtls version of it returns the bytes read on EAGAIN-like conditions.
    IMO, mbedtls behavior is better, to give the caller a chance to perform poll()-wait.

i think the ideal behavior (both fo send and recv) is:

  • on an error, if some bytes have been sent/received already, return the number of bytes
  • otherwise, if the error is EAGAIN equivalent, return 0. (or something distinguishable, like MQTT_ERROR_SOCKET_AGAIN)
  • otherwise, return MQTT_ERROR_SOCKET_ERROR.
@LiamBindle
Copy link
Owner

Thanks for raising this @yamt. Sorry for my delayed response. I like your suggestion. Are you able to make a PR for this?

@LiamBindle LiamBindle added enhancement New feature or request help wanted Extra attention is needed labels Nov 7, 2020
@yamt
Copy link
Contributor Author

yamt commented Nov 13, 2020

i guess i can submit a patch. but the only implementation i can actually test by myself is mbedtls.

yamt added a commit to yamt/MQTT-C that referenced this issue Feb 4, 2022
yamt added a commit to yamt/MQTT-C that referenced this issue Feb 4, 2022
To follow the protocol suggested in
LiamBindle#110
@yamt
Copy link
Contributor Author

yamt commented Feb 4, 2022

mbedtls and unix socket: #156

yamt added a commit to yamt/MQTT-C that referenced this issue Feb 14, 2022
To follow the protocol suggested in
LiamBindle#110
Willieodwyer pushed a commit to Willieodwyer/MQTT-C that referenced this issue Mar 8, 2022
Willieodwyer pushed a commit to Willieodwyer/MQTT-C that referenced this issue Mar 8, 2022
To follow the protocol suggested in
LiamBindle#110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants