Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚀 add 'onListen' application hook #4542

Closed
2 tasks done
Yberion opened this issue Feb 1, 2023 · 16 comments · Fixed by #4899
Closed
2 tasks done

🚀 add 'onListen' application hook #4542

Yberion opened this issue Feb 1, 2023 · 16 comments · Fixed by #4899
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@Yberion
Copy link

Yberion commented Feb 1, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

We have an onReady hook used if we want to trigger things right before the server accept requests.
We don't have a hook for when the server accept requests.

My proposal is to add a new application hook onListen for this purpose.
I think it could act as the onReady application hook.

Motivation

Currently if we want some code to be executed when the server accept requets we need to do something similar to this:

import Fastify from 'fastify';

const PORT = parseInt(process.env.PORT, 10) || 4000;
const HOST = process.env.FASTIFY_LISTEN_HOST || 'localhost';

const fastify = Fastify();

fastify.listen({ port: PORT, host: HOST })
.then((address) => {
  fastify.log.info(`Fastify server listening on http://localhost:${PORT} or ${address}`);

  // /!\ some code that needs the server to accept requests
})
.catch((error) => {
  fastify.log.error(error);

  process.exit(1);
});

If we have a lot of code in this case or if we want to decouple the logic from inside this Promise, we currently cant.

I have a use case where I need to run the server which will request itself and cache the result of the request in a Redis server.

Also I'm not sure about that since I never used it, but it could also be usefull in serverless functions.

Example

We could even create plugins implementing logics with this new application hook (but I don't know how it would act as listen() is waiting for all plugins to be ready).

import Fastify from 'fastify';

const PORT = parseInt(process.env.PORT, 10) || 4000;
const HOST = process.env.FASTIFY_LISTEN_HOST || 'localhost';

const fastify = Fastify();

fastify.addHook('onListen', (done) => {
  // /!\ some code that needs the server to accept requests

  done()
});

fastify.listen({ port: PORT, host: HOST })
.then((address) => {
  fastify.log.info(`Fastify server listening on http://localhost:${PORT} or ${address}`);
})
.catch((error) => {
  fastify.log.error(error);

  process.exit(1);
});
@Yberion
Copy link
Author

Yberion commented Feb 1, 2023

I just saw that we could do:

import Fastify from 'fastify';

const PORT = parseInt(process.env.PORT, 10) || 4000;
const HOST = process.env.FASTIFY_LISTEN_HOST || 'localhost';

const fastify = Fastify();

fastify.server.on('listening', () => {
  // /!\ some code that needs the server to accept requests
  fastify.log.info('test');
});

fastify.listen({ port: PORT, host: HOST })
.then((address) => {
  fastify.log.info(`Fastify server listening on http://localhost:${PORT} or ${address}`);
})
.catch((error) => {
  fastify.log.error(error);

  process.exit(1);
});

Feel free to close this issue if the new application hook is not usefull in that case.

Edit:

It seems that test log is done before the Fastify server listening on https://localhost:4001 or http://[::1]:4001 log, I have no idea if it's intended.

@mcollina
Copy link
Member

mcollina commented Feb 1, 2023

It seems that test log is done before the Fastify server listening on https://localhost:4001 or http://[::1]:4001 log, I have no idea if it's intended.

It's intended, the log line is executed a few microticks later.


This would be a good addition. Would you like to send a PR?

@Yberion
Copy link
Author

Yberion commented Feb 1, 2023

This would be a good addition. Would you like to send a PR?

Sure, I will take a look.

I have some "doubt" as the .listen() will wait for all plugins to be ready, but it should not cause problems.

@Yberion
Copy link
Author

Yberion commented Feb 4, 2023

Hello @mcollina, I've done something locally, but clearly I'm not enough experienced with the JS Fastify base code and unit test standard.

Should I still provide a PR without proper tests (as a draft or something like that) or would it be faster for you to implement it on your side?

@metcoder95
Copy link
Member

Hey! What are the doubts you have?

I think you should start with the basics and provide some testing that verifies the new hook is triggered when it should, validations about the parameters for the hook, and so on. Open a PR (or a draft if you want to verify your test adds) and we can help you from there 🙂

@Eomm Eomm added semver-minor Issue or PR that should land as semver minor and removed enhancement labels Apr 1, 2023
@erfanium
Copy link
Contributor

erfanium commented Apr 16, 2023

@Yberion I have a concern about this idea.
Imagine if this feature were implemented now, how would you be able to correctly test your plugins? In the fastify ecosystem, we never create a real HTTP server; instead, we usually use fastify.inject(...).

Here's what our tests for a plugin look like:

const app = fastify();

beforeAll(async () => {
  await app.register(PluginToBeTested);
  await app.ready();
})

afterAll(async () => {
  await app.close();
})

test('Case', () => {
  const response = await app.inject({ url: '/', method: 'GET' });
  //// ...
})

As you can see, we didn't start a real HTTP server here, which is a really cool feature of Fastify. I really love it.
However, if fastify supported the onListen hook. We could no longer rely on this approach for testing our plugins.

@metcoder95
Copy link
Member

metcoder95 commented Apr 16, 2023

However, if fastify supported the onListen hook. We could no longer rely on this approach for testing our plugins.

That's a pretty valid concern, though the new hook should not alter the current functionality of fastify. Meaning that as being a new implementation it should not have side effects on current features of the framework (e.g. testing).

But the point is valid, how should the plugin behave while testing?

We have either of two options:

  1. Fastify makes its a best-effort to identify when is being called through inject so it calls the onListening hook just after all the plugins are loaded (basically after the ready)
  2. We do nothing about it but document this limitation of the hook.

I'm of course more into the first option, but just wanted to put all the cards over the table 🙂

@erfanium
Copy link
Contributor

erfanium commented Apr 16, 2023

Perhaps if @Yberion provides us with a real-world example of the onListen hook, we could make a better decision.

@Yberion
Copy link
Author

Yberion commented Apr 16, 2023

Hello @metcoder95 & @erfanium,

Sorry for the delay, I'm quite busy nowadays.

This new hook should not block other hooks, it's just a short and more "Fastify" way of calling fastify.server.on('listening', () => { ... });

I'm using it on my project right now on our Angular SSR/SSG server, I request our CMS to retrieve all pages and after that generate the static pages by calling the fastify server and store them in a Redis cache server.
But after retrieving the pages, I need to ensure that fastify is accepting requests as I'm requesting itself (the fastify server that render the Angular pages) for that, everything is done on the same fastify server.

  // We can add logics once the server start accepting requests
  fastify.server.on('listening', () => {
    // Workaround in dev mode, the http proxy middleware from the Angular SSR dev server needs time to initialize.
    // https://angular.io/api/core/isDevMode
    if (isDevMode()) {
      setTimeout(() => {
        startCaching();
      }, 2500); // Good computer
      //}, 5000); // Bad computer
    } else {
      startCaching();
    }
  });

@metcoder95
Copy link
Member

No problem at all!

The use case is similar to do:

await fastify.listen()

if (isDevMode()) {
  setTimeout(() => {
	startCaching();
  }, 2500);
} else {
  startCaching();
}

I'm trying to imagine how plugins can benefit from it, but I don't have a use case coming out.
Though, if still looking for the hook, I believe we should still provide a way to test its usage

@climba03003
Copy link
Member

climba03003 commented Apr 17, 2023

I am thinking is fastify-delay-request enough for such usage.

import FastifyDelayRequest from 'fastify-delay-request'
import { once } from 'events'

// you can wrap it with encapsulation to prevent it 
// from blocking the others API route
fastify.register(FastifyDelayRequest, {
})

fastify.delay.addTask(async function() {
  // add task, like building cache here
  
  // or, you can wait for listening event before others task
  await once(fastify.server, 'listening')
})

In this case, your server is allowed to spin up and accepts requests while waiting for the caching jobs.

@yeeao
Copy link

yeeao commented Jul 6, 2023

Hello @Yberion @erfanium @metcoder95,
My team from CodeDay Labs would like to conribute to this issue. We noticed that there were some discussions about the possible use case/benefits behind this hook and so we wanted to double-check if this issue still open? Is this onListen() hook still needed for the framework?

@metcoder95
Copy link
Member

metcoder95 commented Jul 7, 2023

👋 @yeeao

The issue is still open and we welcome any contribution.
The main benefit we can see for the onListen hook is to have the ability for plugins to execute particular logic whenever the servers start listening for requests, among some other use cases we are not obviating.

Having a PR that proposes its design, is really welcome and can be a good starting point to gather ideas and adjust it accordingly 🙂

@yeeao
Copy link

yeeao commented Jul 7, 2023

Thank you for the clarification and advice!
My team @yeeao @BrendenInhelder @kdrewes will be working on this issue.

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 4, 2023

Before we progress on the PR, we should discuss what we should do with errors. Should we throw and crash the fastify instance, or should we log errors and thats it.

@Eomm wrote to me, that all other application hooks are necessary(!) to start for running the fastify instance, but onListen is not necessary anything. He thinks that logging by default makes most sense and if we want to crash the server because of an error, there should be an option or an errorHandler added to handle that.
Also he asked if there is the possibility to introduce race conditions. Quote:

If the user uses it to cache something to use into its routes. Because if you consider it an application hook, it seems odd this behaviour.

@mcollina wrote to me that ".listen() should error, what ever this means".

@Eomm Eomm linked a pull request Aug 26, 2023 that will close this issue
4 tasks
Copy link

github-actions bot commented Dec 1, 2023

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants