Skip to content

Fix client rx timeout #226

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix client rx timeout #226

wants to merge 7 commits into from

Conversation

TienHuyIoT
Copy link

  • The client RX timeout is always set to 0 (disabled) in the _send() function

- The client RX timeout is always set to 0 (disabled) in the _send()
function
@@ -44,6 +47,10 @@
#define ASYNCWEBSERVER_USE_CHUNK_INFLIGHT 1
#endif

#ifndef ASYNCWEBSERVER_RX_TIMEOUT
#define ASYNCWEBSERVER_RX_TIMEOUT 3 // Seconds for timeout
Copy link
Member

Choose a reason for hiding this comment

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

should be set to 0 (the default in AsyncTCP) for backward compatiblity right ?

Ideally, AsyncTCP should even expose a constant macro that gets reused here

Copy link
Author

@TienHuyIoT TienHuyIoT Jul 14, 2025

Choose a reason for hiding this comment

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

I don’t think so.
The Async WebServer is typically used to continuously listen for new client connections. In the case where a client is connected but no data is exchanged, the server should close the connection after a timeout occurs.

Except connection ws and sse.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but there are some use cases such as long-lived connections or long polling where a normal request would be more like a sse / ws request.

my second point is that it introduces a change compared to the current defaults. If some users rely on the current default, then this change can break their app.

Also, another place where this timeout should be set to zero I think is when we decide to pause a request (there is a pause method in the code).

So more generally, I think it could be better to just leave it to its current defaults and then issue a 3.8.0 release with this newly added feature , and we document in the wiki and release notes that there is a new extra timeout methods people can set.

Copy link
Member

Choose a reason for hiding this comment

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

@me-no-dev : any opinion on that ?

Copy link
Author

Choose a reason for hiding this comment

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

I agree, but there are some use cases such as long-lived connections or long polling where a normal request would be more like a sse / ws request.

In some cases, during file downloads or uploads, long connections may be lost or broken due to network issues. If timeout handling is disabled, the server may keep these connections indefinitely, leading to a memory leak.

my second point is that it introduces a change compared to the current defaults. If some users rely on the current default, then this change can break their app.

This update does not change the design of the implemented Async WebServer; it only fixes incorrect RX timeout handling.

Also, another place where this timeout should be set to zero I think is when we decide to pause a request (there is a pause method in the code).

I understand that point, so the new update has implemented a mechanism to back up and restore the RX timeout.

So more generally, I think it could be better to just leave it to its current defaults and then issue a 3.8.0 release with this newly added feature , and we document in the wiki and release notes that there is a new extra timeout methods people can set.

The timeout method is not new—it is already implemented and based on AsyncTCP. The Async WebServer simply registers the onTimeout() callback and then decides whether to close the connection on its own.
However, 👍 documentation will be useful for maintenance and future improvements to the fantastic Async WebServer library.

Thanks so much for your comments!

Copy link
Member

Choose a reason for hiding this comment

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

OK!
If @me-no-dev is happy with your proposal, then we can merge.

Copy link
Member

Choose a reason for hiding this comment

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

I need a bit of time to check everything out

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