Skip to content
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

Synchronious write stream handling fix #1000

Merged

Conversation

TiARETiK
Copy link
Contributor

I encountered a problem with handling sequential requests. In particular, some of the responses stalled until timeout. After some research, it was found that the reason for such a behavior was my previous pull request, that changed response handling in a way of making all such handling synchronous. While synchronicity itself has not proven problematic, it was found that some of the response management done in do_write is absent in do_write_sync. That is, clearing the response body and the http_connection parsers is not done.

That has been fixed, and bug_quick_repeated_request was change in such a way as to show the problem via staling on previous version and passing on current.

@gittiver
Copy link
Member

Maybe it fixes:
#997

@TiARETiK
Copy link
Contributor Author

TiARETiK commented Feb 20, 2025

@gittiver , I believe this pull request is merge-ready. Could you please look into it?

Copy link
Member

@gittiver gittiver left a comment

Choose a reason for hiding this comment

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

Please see my comment on test, will merge this but this should be fixed in a separate PR maybe.

auto end = std::chrono::high_resolution_clock::now();
std::chrono::seconds testDuration = std::chrono::duration_cast<std::chrono::seconds>(end - start);

CHECK(testDuration.count() < explicitTimeout);
Copy link
Member

Choose a reason for hiding this comment

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

Is a time of one second and 200s really needed for the test?
Lets assume a 100ms delay, then we would at least end up with 20s (worst case).

@gittiver gittiver marked this pull request as ready for review February 20, 2025 17:27
@gittiver gittiver merged commit 7e66299 into CrowCpp:master Feb 20, 2025
11 checks passed
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.

The browser indicates that the process is stalled while attempting to load resources
3 participants