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

Close header fix #103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Close header fix #103

wants to merge 1 commit into from

Conversation

DobromirM
Copy link
Member

@DobromirM DobromirM commented Jan 12, 2023 •
edited
Loading

  • Moved close from didRequest to didRespond as per RFC 2616.
  • Fixed didRespond being called too early.

Fixes #100

@DobromirM
* Moved close from didRequest to didRespond as per RFC 2616.
05b52df 
* Fixed `didRespond` being called too early.
ajay-gov
ajay-gov reviewed Jan 15, 2023
View reviewed changes
Copy link
Member

@ajay-gov ajay-gov left a comment

Choose a reason for hiding this comment

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

The changes to the AbstractHttpServer looks good.

The changes to the Station might impact the performance of the I/O with WebSockets. What is the behavior without this change?

@DobromirM
Copy link
Member Author

DobromirM commented Jan 16, 2023 •
edited
Loading

The changes to the AbstractHttpServer looks good.

The changes to the Station might impact the performance of the I/O with WebSockets. What is the behavior without this change?

didRespond for AbstractHttpServer was being called before the response was actually received by the client, resulting in a server error if the close method is called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajay-gov ajay-gov ajay-gov left review comments

@brohitbrose brohitbrose Awaiting requested review from brohitbrose

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Connection close header error
2 participants
@DobromirM @ajay-gov