Skip to content

Commit

Permalink
refactor: adjust on-connect call
Browse files Browse the repository at this point in the history
  • Loading branch information
metcoder95 committed Nov 22, 2023
1 parent c0cf8bb commit 8b3644e
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 23 deletions.
50 changes: 29 additions & 21 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1665,19 +1665,6 @@ function writeH2 (client, session, request) {
return false
}

try {
// TODO(HTTP/2): Should we call onConnect immediately or on stream ready event?
request.onConnect((err) => {
if (request.aborted || request.completed) {
return
}

errorRequest(client, request, err || new RequestAbortedError())
})
} catch (err) {
errorRequest(client, request, err)
}

if (request.aborted) {
return false
}
Expand All @@ -1689,9 +1676,34 @@ function writeH2 (client, session, request) {
headers[HTTP2_HEADER_AUTHORITY] = host || client[kHost]
headers[HTTP2_HEADER_METHOD] = method

try {
// We are already connected, streams are pending.
// We can call on connect, and wait for abort
request.onConnect((err) => {
if (request.aborted || request.completed) {
return
}

err = err || new RequestAbortedError()

if (stream != null) {
util.destroy(stream, err)

h2State.openStreams -= 1
if (h2State.openStreams === 0) {
session.unref()
}
}

errorRequest(client, request, err)
})
} catch (err) {
errorRequest(client, request, err)
}

if (method === 'CONNECT') {
session.ref()
// we are already connected, streams are pending, first request
// We are already connected, streams are pending, first request
// will create a new stream. We trigger a request to create the stream and wait until
// `ready` event is triggered
// We disabled endStream to allow the user to write to the stream
Expand Down Expand Up @@ -1805,18 +1817,14 @@ function writeH2 (client, session, request) {
})

stream.on('data', (chunk) => {
// Aborting a request does not abort
// the stream, so we need to check and destroy it if request
// is aborted.
if (request.aborted) util.destroy(stream, new RequestAbortedError())

if (request.onData(chunk) === false) stream.pause()
})

stream.once('close', () => {
h2State.openStreams -= 1
// TODO(HTTP/2): unref only if current streams count is 0
if (h2State.openStreams === 0) session.unref()
if (h2State.openStreams === 0) {
session.unref()
}
})

stream.once('error', function (err) {
Expand Down
5 changes: 3 additions & 2 deletions test/fetch/http2.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const { Client, fetch, Headers } = require('../..')

const nodeVersion = Number(process.version.split('v')[1].split('.')[0])

plan(7)
plan(8)

test('[Fetch] Issue#2311', async t => {
const expectedBody = 'hello from client!'
Expand Down Expand Up @@ -438,7 +438,7 @@ test('Issue #2386', async t => {
stream.end(JSON.stringify(expectedResponseBody))
})

t.plan(3)
t.plan(4)

server.listen(0)
await once(server, 'listening')
Expand Down Expand Up @@ -470,6 +470,7 @@ test('Issue #2386', async t => {
)

controller.abort()
t.pass()
} catch (error) {
t.error(error)
}
Expand Down

0 comments on commit 8b3644e

Please sign in to comment.