Skip to content

Wrong HTTPS example fixed. Will save others my #5734 hours of debugging #5739

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

Closed
wants to merge 3 commits into from
Closed

Conversation

joysfera
Copy link

@joysfera joysfera commented Feb 8, 2019

The provided HTTPS examples are misleading and plain wrong because don't ensure the client object lives longer than the https object, which is required per comment hidden in the source code. Otherwise nasty crashes will occur (#5734 ). I have fixed it using a nested code block and a clear comment.

Serial.println(payload);
/* in order to ensure the client object lives the entire time of the HTTPClient
the HTTPClient is in its own nested code block */
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

HTTPClient doesn't need to be in its own code block, it just needs to be instantiated after client. Destructors are called in reverse order, which ensures https gets destroyed first and client second.
I suggest removing the code block, and updating the comment to explain this.

Copy link
Author

Choose a reason for hiding this comment

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

I don't agree. It's way too dangerous to rely on order of code lines. Took me two days to debug that. It should be totally explicit, IMHO.

HTTPClient https;
/* in order to ensure the client object lives the entire time of the HTTPClient
the HTTPClient is in its own nested code block */
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

@d-a-v
Copy link
Collaborator

d-a-v commented May 25, 2019

@joysfera The nested block is not the preferred way but the information or warning is relevant.
Would you update your PR with a comment to inform readers about the requirement ?
Also the last debug line is interesting to have.

@d-a-v d-a-v added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label May 25, 2019
@joysfera
Copy link
Author

I understand you don't want to accept the nested block solution. However, I don't agree with a "solution" based on the order of variable declarations. Adding a comment doesn't really "fix" that.
The whole API is wrong. Possible solution is listed here: #5734 (comment)

@earlephilhower
Copy link
Collaborator

Closing as abandoned with changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants