Skip to content

Conversation

@ONLYstcm
Copy link
Contributor

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

Copy link

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Owner

@gilmaimon gilmaimon left a 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 :)

@ONLYstcm
Copy link
Contributor Author

@gilmaimon please check the new changes as per your review :)

Copy link
Owner

@gilmaimon gilmaimon left a 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 "";
Copy link
Owner

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?

Copy link
Contributor Author

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();
Copy link
Owner

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();
Copy link
Owner

Choose a reason for hiding this comment

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

const uint64_t millisBeforeReadingHeaders

@ONLYstcm
Copy link
Contributor Author

2nd pass of reviews added

@gilmaimon gilmaimon merged commit 558acde into gilmaimon:master Jul 29, 2021
@gilmaimon
Copy link
Owner

Thanks, merged!

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