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: wait a macrotick to resume without pipelining #1465

Merged
merged 2 commits into from May 25, 2022
Merged

fix: wait a macrotick to resume without pipelining #1465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f831dc
Wait a macrotick to resume without pipelining
mcollina May 24, 2022
661ac41
fixup
mcollina May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/client.js
View file
Open in desktop
Expand Up @@ -873,6 +873,11 @@ class Parser {
// have been queued since then.
util.destroy(socket, new InformationalError('reset'))
return constants.ERROR.PAUSED
} else if (client[kPipelining] === 1) {
// We must wait a full event loop cycle to reuse this socket to make sure
// that non-spec compliant servers are not closing the connection even if they
// said they won't.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just nitpicking this comment: I think closing the connection without warning is actually spec-complaint for the server. Servers are free to close connections on a whim, no matter what. Persistence is encouraged where possible, but not strictly required at all times.

From https://httpwg.org/specs/rfc7230.html#rfc.section.6.5:

A client, server, or proxy MAY close the transport connection at any time.

Copy link
Member Author

Choose a reason for hiding this comment

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

The server in the text/example close the connection while sending Connection: Keep-Alive header, which is in contrast with https://datatracker.ietf.org/doc/html/rfc7230#section-6.3/. Basically the server is communicating to the client that it supports persistent connections while it does not as it closes every incoming connection immediately.

Implementing the behavior you want in a spec-compliant way would be to set a Connection: close header. This is already covered in undici.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ronag wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @mcollina

setImmediate(resume, client)
} else {
resume(client)
}
Expand Down
31 changes: 31 additions & 0 deletions test/inflight-and-close.js
View file
Open in desktop
@@ -0,0 +1,31 @@
'use strict'

const t = require('tap')
const { request } = require('..')
const http = require('http')

const server = http.createServer((req, res) => {
res.writeHead(200)
res.end('Response body')
res.socket.end() // Close the connection immediately with every response
}).listen(0, '127.0.0.1', function () {
const url = `http://127.0.0.1:${this.address().port}`
request(url)
.then(({ statusCode, headers, body }) => {
t.pass('first response')
body.resume()
body.on('close', function () {
t.pass('first body closed')
})
return request(url)
.then(({ statusCode, headers, body }) => {
t.pass('second response')
body.resume()
body.on('close', function () {
server.close()
})
})
}).catch((err) => {
t.error(err)
})
})