fix: handle abort signal before server is ready by shayff · Pull Request #4886 · fastify/fastify

shayff · 2023-07-08T14:17:43Z

Resolve #4829

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

shayff · 2023-07-08T14:18:39Z

@metcoder95 Hey, do you think it's good approach? Where do u suggest to put the tests for that?

lib/server.js

Uzlopak · 2023-07-08T14:25:49Z

I know you asked metcoder not me, but you could put them into server.test.js for now. If we come to the conclusion that a separate file is better, we can later split it from there.

Uzlopak · 2023-07-08T15:24:12Z

Unit tests pass, so your code doesnt break the existing implementation. I am looking forward for your unit test(s).

Uzlopak · 2023-07-08T20:10:22Z

I wrote a test based on the reported issue and it does not pass:

test('abort signal should stop the fastify instance', t => {
  t.plan(1)

  const controller = new AbortController()

  const fastify = Fastify()
  fastify.listen({ port: 1234, signal: controller.signal }, (err) => {
    t.error(err)
  })
  controller.abort()
  t.equal(fastify.server.listening, false)
})

shayff · 2023-07-08T20:41:52Z

@Uzlopak You are right, I run the test with out the CR fixes for the onAborted.
It's appear that if I just pass the this.close it's trigger the close function with argument which is not function:

server[closeKey] = function (func) {
    if (func && typeof func !== 'function') {
      throw new AVV_ERR_CALLBACK_NOT_FN(closeKey, typeof func)
    }
   ...  
}

I wrap it again so the argument is undefined and it trigger the close function right.

lib/server.js

test/server.test.js

lib/server.js

Uzlopak

LGTM. Thank you for your investigation and supplying the potential solution.

Lets see what the other maintainers say.

test/server.test.js

lib/server.js

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

test/server.test.js

lib/server.js

docs/Reference/Errors.md

lib/errors.js

Uzlopak · 2023-07-11T09:37:08Z

Why do you approve if we still have to change stuff :D?

shayff · 2023-07-11T15:27:53Z

Tests pass now.

Haw about using another variable which is set when aborted and check in that if condition?

And then trigger the close at the end of the flow?

metcoder95

Why do you approve if we still have to change stuff :D?

Because both options SGTM (either the current or the more explicit one), no strong preference there. Although I made another comment that dismisses my approval 😅

test/server.test.js

Tests missed

test/server.test.js

jsumners

Looks good to me.

lib/server.js

metcoder95

LGTM 👍

Uzlopak · 2023-07-12T22:04:17Z

Who has the guts to press the merge button?

metcoder95 · 2023-07-13T07:53:24Z

Uzlopak · 2023-07-13T08:14:49Z

@shayff
thank you for your contribution.
I know it seems like a small feature and alot of comments and snippets flying around, but listen() is a super critical function, which needs extra attention.

E.g. for me it was the first time working with AbortController and i would not have tackled that issue. But you tackled the issue and found a solution. We refined it together and now after 5 days of hard work we got it merged ;).

So I hope you still liked the experience and that you. are still motivated to contribute in the future :).

Have a nice day ;).

shayff · 2023-07-13T08:33:40Z

@Uzlopak It was a great experience and I can't wait for my next PR. 😀
Thanks everyone for the help

climba03003 · 2023-07-13T08:48:42Z

We need to aware of nodejs/node#48725 is discussing the future of abort signal in node API.
Personally, I feel abort signal and explicit resource management serves different purposes, but there has voice to deprecate the related API.

metcoder95 · 2023-07-13T19:59:37Z

Personally, I feel abort signal and explicit resource management serves different purposes, but there has voice to deprecate the related API.

I have the same feeling. IMO, rather than deprecating, I would prefer to reconsider the expectation when the abortsignal is used. For this case, I would prefer the server just to not start at all, and just ignore the signal afterward. The fact that the signal triggers closes the server feels kinda weird.

github-actions · 2024-07-13T00:23:48Z

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.

handle abort signal before server is ready

8ceb6b2

Uzlopak reviewed Jul 8, 2023

View reviewed changes

lib/server.js Outdated Show resolved Hide resolved

CR fixes for addAbortSignalOption

e2ea614

add test

bdfa7b4

fix test

f2a8d01

Uzlopak reviewed Jul 8, 2023

View reviewed changes

lib/server.js Outdated Show resolved Hide resolved

Uzlopak reviewed Jul 8, 2023

View reviewed changes

test/server.test.js Outdated Show resolved Hide resolved

shaypep added 2 commits July 9, 2023 00:02

skip test for lt 15.6.0

4d7d508

remove add abort function

952aa8c

Uzlopak reviewed Jul 8, 2023

View reviewed changes

lib/server.js Outdated Show resolved Hide resolved

move onAborted

d9a546f

Uzlopak approved these changes Jul 8, 2023

View reviewed changes

Uzlopak requested review from mcollina and metcoder95 July 8, 2023 21:50

Fdawgs changed the title ~~handle abort signal before server is ready~~ fix: handle abort signal before server is ready Jul 9, 2023

Fdawgs added the bugfix Issue or PR that should land as semver patch label Jul 9, 2023

metcoder95 reviewed Jul 9, 2023

View reviewed changes

test/server.test.js Outdated Show resolved Hide resolved

test/server.test.js Show resolved Hide resolved

lib/server.js Show resolved Hide resolved

Update test/server.test.js

49b5da9

Co-authored-by: Carlos Fuentes <me@metcoder.dev>

Uzlopak reviewed Jul 9, 2023

View reviewed changes

test/server.test.js Show resolved Hide resolved

Uzlopak reviewed Jul 9, 2023

View reviewed changes

lib/server.js Show resolved Hide resolved

shaypep added 2 commits July 9, 2023 21:28

add server options error

175ae37

change error to listen options

7d06481

Uzlopak reviewed Jul 9, 2023

View reviewed changes

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

Update docs/Reference/Errors.md

e15387b

Uzlopak reviewed Jul 9, 2023

View reviewed changes

lib/errors.js Outdated Show resolved Hide resolved

Uzlopak requested a review from jsumners July 11, 2023 09:37

metcoder95 reviewed Jul 11, 2023

View reviewed changes

test/server.test.js Show resolved Hide resolved

add assertions for onClose hook

4c8518a

shayff requested a review from metcoder95 July 11, 2023 20:41

jsumners reviewed Jul 11, 2023

View reviewed changes

test/server.test.js Outdated Show resolved Hide resolved

wrap tests

5828505

shayff requested a review from jsumners July 11, 2023 21:03

jsumners approved these changes Jul 11, 2023

View reviewed changes

metcoder95 reviewed Jul 11, 2023

View reviewed changes

lib/server.js Show resolved Hide resolved

remove unsed removeEventListener

b362bf9

shayff requested a review from metcoder95 July 12, 2023 18:22

Merge branch 'main' into abort-signal-before-listening

d18eceb

metcoder95 approved these changes Jul 12, 2023

View reviewed changes

metcoder95 merged commit 1c50da3 into fastify:main Jul 13, 2023

renovate bot mentioned this pull request Aug 2, 2023

chore(deps): update all non-major dependencies dan-online/luna#26

Open

1 task

renovate bot mentioned this pull request Sep 15, 2023

chore(integration-tests): update dependency nock to v13.4.0 gatsby-uc/plugins#441

Merged

1 task

github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2024

Uh oh!

Conversation

shayff commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

shayff commented Jul 8, 2023

Uh oh!

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

shayff commented Jul 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uzlopak commented Jul 11, 2023

Uh oh!

shayff commented Jul 11, 2023

Uh oh!

metcoder95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Jul 12, 2023

Uh oh!

metcoder95 commented Jul 13, 2023

Uh oh!

Uzlopak commented Jul 13, 2023

Uh oh!

shayff commented Jul 13, 2023

Uh oh!

climba03003 commented Jul 13, 2023

Uh oh!

metcoder95 commented Jul 13, 2023

Uh oh!

github-actions bot commented Jul 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shayff commented Jul 8, 2023 •

edited

Loading

metcoder95 left a comment •

edited

Loading