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

Implement Connection persistence per RFC 9112, Section 9.3 #913

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

Conversation

rossabaker
Copy link
Member

We are aggressively closing connections in the presence of Connection headers composed exclusively of options other than keep-alive or close. This is contrary to the RFC

Copy link
Member Author

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Sharing for early feedback on the approach before I figure out how to test it.

@@ -83,6 +85,32 @@ private[blaze] trait Http1Stage[F[_]] { self: TailStage[ByteBuffer] =>
true
}

private[http4s] final def checkRequestCloseConnection(req: Request[F]): Boolean = {
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to fix the Boolean blindness, but it ripples to many places.

@@ -67,6 +68,7 @@ private[blaze] trait Http1Stage[F[_]] { self: TailStage[ByteBuffer] =>
protected def contentComplete(): Boolean

/** Check Connection header and add applicable headers to response */
@deprecated("Use checkRequestCloseConnection(Request) instead", "0.23.17")
Copy link
Member Author

Choose a reason for hiding this comment

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

Kept for binary compatibility, though we could do the MiMa dance.

else "\r\n")
closeOnFinish match {
case true if respConn.isEmpty =>
rr << "Connection: close\r\n\r\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a buried side effect in the old code.

@milessabin
Copy link

LGTM.

Maybe a bit off-topic for this PR, but when returning a Connection: close do you pause before closing to allow the client to do an active close avoiding TIME_WAIT on the server side?

@rossabaker
Copy link
Member Author

Hell, I started from the wrong branch.

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.

2 participants