fix: added a check to prevent creation of secondary server when external server is supplied by aarontravass · Pull Request #4741 · fastify/fastify

aarontravass · 2023-05-10T22:51:53Z

As referenced in Issue #4725, this PR attempts to fix that bug.

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

NOTE: Tests seem to fail on Windows but I ran it on Linux and they passed. Not sure why they fail on windows.

lib/server.js

aarontravass · 2023-05-12T21:15:56Z

What tests should I write that can cover this case? I am thinking something similar to this

test('Should accept user defined serverFactory', async t => {
  const server = http.createServer(() => {})
  const app = Fastify({
    serverFactory: () => server
  })
  t.resolves(await app.listen({ port: 0 }))
})

metcoder95 · 2023-05-12T21:36:46Z

What tests should I write that can cover this case? I am thinking something similar to this

test('Should accept user defined serverFactory', async t => {
  const server = http.createServer(() => {})
  const app = Fastify({
    serverFactory: () => server
  })
  t.resolves(await app.listen({ port: 0 }))
})

One that tests the implementation throws when the first server instance is already listening. And a second that intercepts the warning message the second service is already listening

aarontravass · 2023-05-13T22:10:34Z

One that tests the implementation throws when the first server instance is already listening. And a second that intercepts the warning message the second service is already listening

I wrote the test for checking the warning and it works fine but the test for throwing the error keeps timing out. Here's what I've written

test('Should accept user defined serverFactory and throw an error if the supplied server is listening', async t => {
  const server = http.createServer(() => {})
  server.listen()
  const app = await Fastify({
    serverFactory: () => server
  })
  t.rejects(
    async () => {
      await app.listen({ port: 0 })
    }
  )
})

This is the stack for the time out error

 FAIL  test/custom-http-server.test.js
 ✖ timeout!

  test: TAP
  signal: SIGTERM
  handles:
    - type: Server
      events:
        - request
        - connection
        - clientError
      connectionKey: 6::::0
  expired: TAP
  stack: |
    emit (node_modules/signal-exit/index.js:105:13)
    process.listener (node_modules/signal-exit/index.js:123:9)

 FAIL  test/custom-http-server.test.js 1 failed of 10 28s
 ✖ timeout!

Any idea what's going wrong?

metcoder95 · 2023-05-14T13:28:00Z

I’m guessing here, but might be because you are not tearing down the first server that is already listening.
have you tried with:

test('Should accept user defined serverFactory and throw an error if the supplied server is listening', async t => {
  const server = http.createServer(() => {})
  server.listen()
  t.teardown(() => new Promise(res => server.close(res)))
  const app = await Fastify({
    serverFactory: () => server
  })
  t.rejects(
    async () => {
      await app.listen({ port: 0 })
    }
  )
})

metcoder95 · 2023-05-14T13:28:25Z

Please adjust it properly as I made it quicker 😅

aarontravass · 2023-05-14T17:25:05Z

Please adjust it properly as I made it quicker 😅

yep, it worked. I am tearing down the server in both tests, so it doesn't timeout. Pushed my changes

lib/errors.js

lib/server.js

lib/warnings.js

lib/errors.js

lib/server.js

lib/warnings.js

aarontravass · 2023-05-16T17:16:42Z

Looks like ubuntu 16 ci is failing. 404 test is timing out and this happens occasionally on my computer as well.

mcollina

lgtm

ivan-tymoshenko

lgtm

mcollina

After looking at it again, this is going to break a use case of restartable pretty badly: it would prevent a server to be reused when passed in serverFactory.

We should avoid the creation of the secondary server when createServer is supplied and not throw.

metcoder95 · 2023-06-07T07:54:40Z

It seems the linting check is failing.

Besides that, the PR is good to go?

cc: @aarontravass @climba03003 @mcollina

…nal server is passed to serverfactory fastify#4725

…ther than the options

…ed server is listening, then the first server throws an error. added test cases for the same

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

…NING and changed the message as well

…reating secondary server

ivan-tymoshenko

lgtm

mcollina

lgtm

Signed-off-by: Matteo Collina <hello@matteocollina.com>

github-actions · 2024-06-11T00:22:46Z

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.

climba03003 requested changes May 11, 2023

View reviewed changes

lib/server.js Outdated Show resolved Hide resolved

metcoder95 linked an issue May 12, 2023 that may be closed by this pull request

creating multipleBindings function does not catch/handle errors #4730

Closed

2 tasks

aarontravass requested a review from climba03003 May 14, 2023 17:24

metcoder95 reviewed May 14, 2023

View reviewed changes

lib/errors.js Outdated Show resolved Hide resolved

lib/server.js Outdated Show resolved Hide resolved

lib/server.js Outdated Show resolved Hide resolved

lib/warnings.js Outdated Show resolved Hide resolved

aarontravass requested a review from metcoder95 May 14, 2023 21:17

metcoder95 reviewed May 15, 2023

View reviewed changes

lib/errors.js Outdated Show resolved Hide resolved

lib/server.js Outdated Show resolved Hide resolved

lib/warnings.js Outdated Show resolved Hide resolved

mcollina approved these changes May 18, 2023

View reviewed changes

mcollina requested a review from ivan-tymoshenko May 18, 2023 13:02

ivan-tymoshenko approved these changes May 18, 2023

View reviewed changes

climba03003 approved these changes May 18, 2023

View reviewed changes

metcoder95 approved these changes May 18, 2023

View reviewed changes

mcollina requested changes May 19, 2023

View reviewed changes

aarontravass requested a review from mcollina May 21, 2023 20:13

metcoder95 approved these changes May 22, 2023

View reviewed changes

aarontravass and others added 7 commits June 9, 2023 09:54

fix: added a check to prevent creation of secondary server when exter…

6b7350e

…nal server is passed to serverfactory fastify#4725

fix: serverFactoryListening now uses the serverfactory if supplied ra…

656b7dc

…ther than the options

fix: added warning if secondary server is being created and if suppli…

5125051

…ed server is listening, then the first server throws an error. added test cases for the same

Update lib/server.js

495c156

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

fix: rewords FST_ERR_DUPLICATE_SERVER to FST_ERR_SERVER_ALREADY_LISTE…

c1252d5

…NING and changed the message as well

fix: changed warning label, error message

0b16da4

fix: serverFactory usage does not throw an error and instead avoids c…

9db75ba

…reating secondary server

metcoder95 force-pushed the aaron/serverfactory branch from fcc74ee to 9db75ba Compare June 9, 2023 07:55

ivan-tymoshenko approved these changes Jun 9, 2023

View reviewed changes

mcollina approved these changes Jun 9, 2023

View reviewed changes

Merge branch 'main' into aaron/serverfactory

dfec851

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina merged commit 3ec2651 into fastify:main Jun 9, 2023

renovate bot mentioned this pull request Jun 30, 2023

chore(deps): update all non-major dependencies PKUHPC/OpenSCOW#703

Merged

1 task

renovate bot mentioned this pull request Oct 19, 2023

chore(deps): update all non-major dependencies dan-online/bunnycdn-stream-demo#27

Merged

1 task

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

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

metcoder95 commented May 12, 2023

Uh oh!

aarontravass commented May 13, 2023

Uh oh!

metcoder95 commented May 14, 2023

Uh oh!

metcoder95 commented May 14, 2023

Uh oh!

aarontravass commented May 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aarontravass commented May 16, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

metcoder95 commented Jun 7, 2023

Uh oh!

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aarontravass commented May 10, 2023 •

edited

Loading

aarontravass commented May 12, 2023 •

edited

Loading