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.

Sign up for GitHub

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

Jump to bottom

Fix https timeout #1067

Closed
wants to merge 1 commit into from
Closed

Fix https timeout #1067

wants to merge 1 commit into from

Conversation

Shine-neko
Copy link
Contributor

fix #787

@CLAassistant
Copy link

CLAassistant commented Jan 31, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

Keksoj
Keksoj suggested changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@Keksoj Keksoj left a comment

Choose a reason for hiding this comment

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

this is good. can you please change the commit message to be a bit more explicit? for instance, start_https_worker: send empty listeners or similarly

@Shine-neko Shine-neko requested a review from Keksoj January 31, 2024 10:23
Keksoj
Keksoj approved these changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@Keksoj Keksoj left a comment

Choose a reason for hiding this comment

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

great, thanks!

@Keksoj
Copy link
Contributor

Keksoj commented Jan 31, 2024

you have renamed the commit start_https_worker: send empty listeners or similarly please remove or similarly @Shine-neko

@Shine-neko Shine-neko requested a review from Keksoj January 31, 2024 16:04
@FlorentinDUBOIS
Copy link
Collaborator

@Wonshtrum could you take a look at it?

@Wonshtrum Wonshtrum mentioned this pull request Feb 2, 2024
Merged
@Wonshtrum
Copy link
Member

Hi @Shine-neko thanks for contributing to Sozu and pointing out the missing send_listeners (which blocks the server start). I'm sorry we closed your original issue without proper investigation.
I revisited this old code we never use and found that the TCP proxy had the same problem. To avoid future problems I merged the bulk of the 3 start_*_worker functions in prebuild_server in #1068. I added you as a co-author I hope this is fine.

@Keksoj
Copy link
Contributor

Keksoj commented Feb 2, 2024

Closing this for the issue is fixed by #1068
Thanks for raising our awareness on the issue and your contributing, @Shine-neko !

@Keksoj Keksoj closed this Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlorentinDUBOIS FlorentinDUBOIS Awaiting requested review from FlorentinDUBOIS FlorentinDUBOIS is a code owner

@Wonshtrum Wonshtrum Awaiting requested review from Wonshtrum Wonshtrum is a code owner

@Keksoj Keksoj Awaiting requested review from Keksoj

Assignees

@Wonshtrum Wonshtrum

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTPS server timeout
5 participants
@Shine-neko @CLAassistant @Keksoj @FlorentinDUBOIS @Wonshtrum