Skip to content

Conversation

@thunderbirds66
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vareddy-zz
Copy link
Contributor

vareddy-zz commented Apr 24, 2018

Hi @thunderbirds66,
Thank you for your interest in AWS IoT!
If you look at these lines, you can see that ConnectTCPSocket already closes the socket if there was a failure in connect. Hence this change is redundant.
Please let us know if you have any more questions.
Thanks!
Varun

@thunderbirds66
Copy link
Contributor Author

Hi @vareddy, thanks for quick review.

I think my PR is not redundant because socket will not be closed if gethostbyname failed.
We are experiencing this situation in unstable 3G environment.
Thanks!
TB66

@vareddy-zz
Copy link
Contributor

vareddy-zz commented Apr 27, 2018

Hi @thunderbirds66,
I believe you are correct. Thanks for the clarification. To make the code more consistent, could you modify your pull request to remove these lines as well, so that the closing of the socket happens only on the return of ConnectTCPSocket(). Closing the socket twice could lead to instability as the socket is made invalid after closing it once.
After that it would be good to go.
Thanks!
Varun

@thunderbirds66
Copy link
Contributor Author

Hi @vareddy thanks for quick reply!
I think you're right and I modified my PR.
Please review this again.
Thanks!
TB66

@vareddy-zz vareddy-zz merged commit d4cdd25 into aws:master Apr 30, 2018
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.

3 participants