Skip to content

Check if stream is writable to avoid infinite loop in write_content_chunked while using SSE #1478

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

Closed
wants to merge 2 commits into from

Conversation

aldoshkind
Copy link

Hi @yhirose ! Here is PR which fixes issue described in #1475

As you asked here is the copy of issue description:

I am using server-sent events and faced infinite loop. In my case infinite loop occurs when I reopen web-page/web-browser several times.
How to reproduce: run SSE example ssesvr.cc, open browser and reload page several times (5 or 6 in my case on 8-core processor). Voila, page not loading!
Infinite loop is here: https://github.com/yhirose/cpp-httplib/blob/master/httplib.h#L3764
My hotfix looks like this: while (data_available && !is_shutting_down() && strm.is_writable()) { (add && strm.is_writable()). Not sure it's absolutely correct, so no PR from me :) Also I see there are similar pieces of code, maybe they are affected too.
Please investigate the problem!
Thanks!

@yhirose
Copy link
Owner

yhirose commented Feb 2, 2023

@aldoshkind thanks for the pull request. The change looks good to me. Could you do the same at L3692 as well?

@aldoshkind
Copy link
Author

I added same check on lines L3692 and L3646 and tested all three modes manually.
I got new questions while reading the code:

  • shouldn't we return false if stream isn't writable anymore?
  • shouldn't we set error in write_content_without_length like in write_content_chunked and write_content?
  • if so, which error should we return? What does Error::Connection mean? It would be awesome to have comments on all the errors :)

@yhirose
Copy link
Owner

yhirose commented Feb 4, 2023

@aldoshkind thanks for the changes and thoughtful questions. When I looked at your first question with your changes, it turns out that DataSink::is_writable() method isn't necessary and the 'writable' check should be done in DataSink::write() with strm.is_writable(). By doing this, if the write stream becomes unable, the client returns Error::Write.

Regarding your second question, write_content_without_length doesn't need error, since it's only used in Server. Error is only available for Client.

I confirmed that httplib.h in #1483 fixes the SSE problems on my MacBook. When you have time could you test the version on your machine? Thanks a lot!

@aldoshkind
Copy link
Author

@yhirose I checked #1483 , looks like everything works nice, thanks! :)

Nevertheless, I think checking strm.is_writable() in write_content_chunked and its friends is still good idea as it will prevent infinite loop even if user returns from content_provider callback without calling DataSink::write() for some reason. Kind of additional protection level :)

@yhirose
Copy link
Owner

yhirose commented Feb 4, 2023

@aldoshkind you are right. I made the change. Thanks for your fine contribution and suggestion!

@aldoshkind
Copy link
Author

@yhirose Thank you for rapid response and awesome lib :)

ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
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.

2 participants