Skip to content

fix: double hook execution#6013

Merged
mcollina merged 13 commits intomainfrom
double-hook
Mar 24, 2025
Merged

fix: double hook execution#6013
mcollina merged 13 commits intomainfrom
double-hook

Conversation

@Eomm
Copy link
Member

@Eomm Eomm commented Mar 8, 2025

Continuing the discussion about this issue:

To replicate the issue:

  1. follow these instructions: onSend hook can be called twice #4959 (comment)
  2. run npx autocannon -m PUT -c 1 -d 1 http://localhost:3000/ -b '{ "test": "me" }' -H "Content-Type: application/json" (it does not happen always, but 2/3 or run fails for me)

From my experiment, when we resolve the thenable:

fulfilled()

The output of this line:
console.log('fulfilled: ', this.raw.writableEnded, this.raw.destroyed, this.raw.aborted, err) is:

  • working: fulfilled: true true undefined undefined
  • crash: fulfilled: false true undefined Error [ERR_STREAM_PREMATURE_CLOSE]: Premature close

By reading the comments, I see the ERR_STREAM_PREMATURE_CLOSE error is skipped because it is set by eos

fastify/lib/reply.js

Lines 464 to 465 in 30b8129

// We must not treat ERR_STREAM_PREMATURE_CLOSE as
// an error because it is created by eos, not by the stream.

Should we?

Checklist

@mcollina
Copy link
Member

mcollina commented Mar 8, 2025

I'm good with the change. Can you add a test?

@Eomm
Copy link
Member Author

Eomm commented Mar 16, 2025

A step forward:

I can replicate the error by using a single HTTP request by using this client:

const http = require('http')

const postData = JSON.stringify({ test: 'me' })

const options = {
  hostname: 'localhost',
  port: 3000,
  path: '/',
  method: 'PUT',
  headers: {
    'Content-Type': 'application/json',
    'Content-Length': Buffer.byteLength(postData),
  }
}

const req = http.request(options, (res) => {
  console.log('Response received')
})

// Kill the socket immediately (before sending data)
req.on('socket', (socket) => {
  console.log('Destroying socket...')
  setTimeout(() => {
    socket.destroy()
  }, 5)
})

req.on('error', (err) => {
  console.error('Request error:', err)
})

req.write(postData)
req.end()

I will find what we are missing..

image

@Eomm Eomm linked an issue Mar 16, 2025 that may be closed by this pull request
2 tasks
@Eomm Eomm marked this pull request as ready for review March 16, 2025 19:52
@Eomm Eomm added the bugfix Issue or PR that should land as semver patch label Mar 17, 2025
Eomm added 3 commits March 17, 2025 20:31
Signed-off-by: Manuel Spigolon <behemoth89@gmail.com>
Signed-off-by: Manuel Spigolon <behemoth89@gmail.com>
@Eomm
Copy link
Member Author

Eomm commented Mar 22, 2025

checking the flakyness

@Eomm
Copy link
Member Author

Eomm commented Mar 22, 2025

@mcollina would you mind testing this PR in your env since you were effected by the previous fix?

@Eomm Eomm mentioned this pull request Mar 22, 2025
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

my regression test is passing, we can land and ship this.

@mcollina mcollina merged commit 0dfa46c into main Mar 24, 2025
27 checks passed
@mcollina mcollina deleted the double-hook branch March 24, 2025 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Issue or PR that should land as semver patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

onSend hook can be called twice

5 participants