fix(#4774): close bindings through the hook system by metcoder95 · Pull Request #4776 · fastify/fastify

metcoder95 · 2023-05-29T10:33:09Z

The PR attempts to fix an unexpected behaviour when secondary binding servers were not in sync with the lifecycle hooks of the application.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

metcoder95 · 2023-05-29T10:34:47Z

Tests are pending, but @Eomm could you please take a look? Let me know if I might be missing something or if there is a better way you can think off to fix it 🙂

lib/server.js

Eomm

By using

  fastify.close((error) => {
    console.log('Server is closed', error)
  })

this PR fixes the issue

Instead, by using the fastify.server.close, it does not.

I think that focusing on 1 single issue is good

lib/server.js

metcoder95 · 2023-05-29T21:15:31Z

By using
  fastify.close((error) => {
    console.log('Server is closed', error)
  })
this PR fixes the issue

Instead, by using the fastify.server.close, it does not.

I think that focusing on 1 single issue is good

Sure! The issue that is caused fo fastify.server.close seems counter-intuitive for sure. Though is not recommended to close the application in that way as the special handling of keep-alive requests among some other features will be bypassed by doing that. Shall we extend the documentation to recommend so? Maybe that can close some gaps?

Eomm · 2023-05-30T09:53:49Z

Shall we extend the documentation to recommend so? Maybe that can close some gaps?

Yeah, maybe we could expand this https://github.com/fastify/fastify/blob/main/docs/Reference/Server.md#server a bit

"If utilized improperly, certain Fastify features could be disrupted. It is recommended to only use it for attaching listeners."

docs/Reference/Server.md

Co-authored-by: James Sumners <james@sumners.email>

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Eomm

One test is failing - could you check it?

ok 2 - Should not return 503 while closing - pipelining - return503OnClosing: false, skip Node >= v19.x # SKIP

Subtest: Should close the socket abruptly - pipelining - return503OnClosing: false, skip Node < v19.x
   ok 1 - should be equal
   not ok 2 - should be equal
     ---
     compare: ===
     at:
       line: 95
       column: 5
       file: test/close-pipelining.test.js
       type: Test
     stack: |
       Test.<anonymous> (test/close-pipelining.test.js:95:5)
     source: |2
         t.equal(responses[0].status, 'fulfilled')
         t.equal(responses[1].status, 'rejected')
       ----^
         t.equal(responses[2].status, 'rejected')
     diff: |
       --- expected
       +++ actual
       @@ -1,1 +1,1 @@
       -rejected
       +fulfilled
     ...
   
   ok 3 - should be equal
   1..3
   # failed 1 of 3 tests
not ok 3 - Should close the socket abruptly - pipelining - return503OnClosing: false, skip Node < v19.x # time=28.369ms

metcoder95 · 2023-06-19T10:18:48Z

test/close-pipelining.test.js

+
  t.equal(responses[0].status, 'fulfilled')
-  t.equal(responses[1].status, 'rejected')
+  t.equal(responses[1].status, 'fulfilled')


It seems that, with the pipelining, and the adjustments to the way we handle the secondary bindings, always at least 2 requests made it through, and from the third one on are rejected.

metcoder95 · 2023-06-19T10:19:35Z

Done in 9466075, can you verify @Eomm 🙂

github-actions · 2024-06-23T00:25:05Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jsumners reviewed May 29, 2023

View reviewed changes

lib/server.js Outdated Show resolved Hide resolved

Eomm reviewed May 29, 2023

View reviewed changes

lib/server.js Outdated Show resolved Hide resolved

Eomm reviewed Jun 1, 2023

View reviewed changes

docs/Reference/Server.md Outdated Show resolved Hide resolved

metcoder95 and others added 5 commits June 14, 2023 10:19

fix: close bindings through the hook system

e7f5625

docs: replace comments

dc7388b

Co-authored-by: James Sumners <james@sumners.email>

refactor: shallow error for hook

2856aa0

docs: add warning about Server usage

8e13a3f

fix: bad quoting

07477a5

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

metcoder95 force-pushed the fix/4774 branch from 72b54a4 to 07477a5 Compare June 14, 2023 08:21

test: add proper testing

9e0d491

metcoder95 marked this pull request as ready for review June 14, 2023 11:30

metcoder95 requested review from Eomm and jsumners June 14, 2023 11:30

test: improve scenarios

60edabc

jsumners approved these changes Jun 14, 2023

View reviewed changes

Merge branch 'main' into fix/4774

73ee3dd

Eomm reviewed Jun 19, 2023

View reviewed changes

test: adjust pipelining test

9466075

metcoder95 commented Jun 19, 2023

View reviewed changes

Eomm approved these changes Jun 19, 2023

View reviewed changes

metcoder95 merged commit c4b8d85 into main Jun 23, 2023

metcoder95 deleted the fix/4774 branch June 23, 2023 13:53

github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(#4774): close bindings through the hook system#4776

fix(#4774): close bindings through the hook system#4776
metcoder95 merged 9 commits intomainfrom
fix/4774

metcoder95 commented May 29, 2023 •

edited

Loading

Uh oh!

metcoder95 commented May 29, 2023

Uh oh!

Uh oh!

Eomm left a comment

Uh oh!

Uh oh!

metcoder95 commented May 29, 2023 •

edited

Loading

Uh oh!

Eomm commented May 30, 2023

Uh oh!

Uh oh!

Eomm left a comment

Uh oh!

metcoder95 Jun 19, 2023

Uh oh!

metcoder95 commented Jun 19, 2023

Uh oh!

github-actions bot commented Jun 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

metcoder95 commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

metcoder95 commented May 29, 2023

Uh oh!

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metcoder95 commented May 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eomm commented May 30, 2023

Uh oh!

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Subtest: Should close the socket abruptly - pipelining - return503OnClosing: false, skip Node < v19.x

Uh oh!

metcoder95 Jun 19, 2023

Choose a reason for hiding this comment

Uh oh!

metcoder95 commented Jun 19, 2023

Uh oh!

github-actions bot commented Jun 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

metcoder95 commented May 29, 2023 •

edited

Loading

metcoder95 commented May 29, 2023 •

edited

Loading