Skip to content

Do not close socket in case of protocol upgrade #2132

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 1 commit into
base: master
Choose a base branch
from

Conversation

snakelizzard
Copy link

Http should not processing should stop at this point, socket will be reused for another protocol.

Http should not processing should stop at this point,
socket will be reused for another protocol.
@yhirose
Copy link
Owner

yhirose commented Apr 19, 2025

@snakelizzard thanks for the pull request. Could you please add at least one unit test in test/test.cc? Then, I'll review the code. Thanks!

@yhirose
Copy link
Owner

yhirose commented May 9, 2025

@falbrechtskirchinger, @PixlRainbow I am thinking of closing the pull request. Do you have any thoughts? Thanks!

@PixlRainbow
Copy link
Contributor

@snakelizzard I would expect that to fit within the "style" of httplib, the socket should be kept alive by blocking the thread from a content IO callback passed to httplib, instead of "handing off" the socket after the httplib thread exits; the protocol implementation is called from within that callback.
However, I am unsure of how @falbrechtskirchinger's planned switching protocol support is implemented.

@snakelizzard
Copy link
Author

Hello.

Sorry, I have not replied about unit tests from the beginning. This is related to @PixlRainbow note on style. I feel my solution is a bit hacky and does not follow the idea of the project. This is what I achieved by 20 min debugging when I realized httplib does not support protocol switch. I feel the value of my patch to publish a quick and dirty solution to whatever is agreed to take it on their own risk.

To make it right, I need better understand the code. And create unit tests. Unfortunately, I lack time to do all this. Please just close PR if it does not fit into @falbrechtskirchinger plans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants