-
Notifications
You must be signed in to change notification settings - Fork 109
Added 1 second timeout to read data in readLine #104
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
Conversation
3imed-jaberi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
gilmaimon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for the delay.
I posted some review comments on the code, once those will be refactored I'll merge :)
src/tiny_websockets/network/generic_esp/generic_esp_clients.hpp
Outdated
Show resolved
Hide resolved
src/tiny_websockets/network/generic_esp/generic_esp_clients.hpp
Outdated
Show resolved
Hide resolved
|
@gilmaimon please check the new changes as per your review :) |
gilmaimon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ONLYstcm Here are some more tiny comments. I think you forgot to remove on of the lines that use _timer so that file will probably not compile.. Also, see 2 tiny const comments.
Thanks!
|
|
||
| uint64_t millisBeforeReadingHeaders = millis(); | ||
| while( ch != '\n' && available()) { | ||
| if ( millis() - _timer > readLineTimeout) return ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you need to remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies I pushed the wrong commit, this was supposed to be removed
|
|
||
| int ch = -1; | ||
|
|
||
| uint64_t millisBeforeReadingHeaders = millis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const uint64_t millisBeforeReadingHeaders
|
|
||
| int ch = -1; | ||
|
|
||
| uint64_t millisBeforeReadingHeaders = millis(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const uint64_t millisBeforeReadingHeaders
|
2nd pass of reviews added |
|
Thanks, merged! |
A simple 1 second connection timeout limit that will abort a connection request if a response is not received in time to address Issue 78. Tested with ESP32