Skip to content

Conversation

@fabianfett
Copy link
Member

Motivation

  • It is unclear what it means to cancel a connection

Modifications

  • Rename cancel connection to shutdown, with an explicit code comment
  • Add .http1_1 and .http2 cases to HTTPConnectionPool.Connection

@fabianfettfabianfett requested review from Lukasa and glbrnttJuly 23, 2021 08:07
@fabianfettfabianfett changed the title Cleanup: Cancel -> ShutdownCleanup: Connection cancel -> shutdownJul 23, 2021
@fabianfettfabianfett added the 🔨 semver/patch No public API change. label Jul 23, 2021
@fabianfettfabianfett added this to the HTTP/2 support milestone Jul 23, 2021
Copy link
Collaborator

@glbrnttglbrntt left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment on lines 66 to 68
return connection.execute(request: request)
case.http2(let connection):
return connection.executeRequest(request)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's some mildly irritating asymmetry :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed.

}

fileprivatefunc cancel(){
/// Closes the connection without cancelling running requests.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens to the requests if we don't cancel them? Why would do use this over shutdown?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In the pool state machine, we know if a connection is idle or not. For this reason, we can use the direct way to close, if we are sure nothing is running on the connection.

@LukasaLukasa merged commit 44efb94 into swift-server:mainJul 23, 2021
@fabianfettfabianfett deleted the ff-cleanup-shutdown branch July 23, 2021 12:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patchNo public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@fabianfett@Lukasa@glbrntt