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 preamble service that runs closure before running service #200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adam-fowler
Copy link
Member

@adam-fowler adam-fowler commented Mar 10, 2025

I find I am writing this small helper service across many applications, so thought it might be of use as part of the library. Basically it is a service that runs a preamble closure before running a child service. eg

let serviceGroup = ServiceGroup(
    services: [
        databaseService,
        PreambleService(service: webserver) { try await performDatabaseMigrations() }
    ],
    logger: logger
)

I have also extended it to take an array of services if the child service is a ServiceGroup

let serviceGroup = ServiceGroup(
    services: [
        databaseService,
        PreambleService(services: [webserver, jobQueue]) { try await performDatabaseMigrations() }
    ],
    logger: logger
)

Struggling to find a name for the type that I'm happy with though. This was the best I could come up with.

@czechboy0
Copy link
Collaborator

Hi @adam-fowler,

do you use this pattern because the contents of the preamble closure need to run inside a ServiceGroup, for example because they use graceful shutdown?

I assume there's a reason this wouldn't work for you?

try await performDatabaseMigrations()
let serviceGroup = ServiceGroup(
    services: [
        webserver
    ],
    logger: logger
)

@FranzBusch
Copy link
Contributor

I think we can model this a bit differently to support Services either having finished or expressing some state before we start the next services. Though one thing that I'm thinking of more and more lately is that all of this can be expressed using with-style methods. Maybe its time we provide ways to tie in with-style methods into the graceful shutdown ordering.

@adam-fowler
Copy link
Member Author

adam-fowler commented Mar 10, 2025

do you use this pattern because the contents of the preamble closure need to run inside a ServiceGroup, for example because they use graceful shutdown?

I assume there's a reason this wouldn't work for you?

The database is also a service. So it needs to be running as well.

@adam-fowler
Copy link
Member Author

I think we can model this a bit differently to support Services either having finished or expressing some state before we start the next services. Though one thing that I'm thinking of more and more lately is that all of this can be expressed using with-style methods. Maybe its time we provide ways to tie in with-style methods into the graceful shutdown ordering.

Can you give us a code example to show what you mean. with-style methods tend to be used for scoping resources to the confines of a closure. I can't see how that works in an array of services.

@czechboy0
Copy link
Collaborator

czechboy0 commented Mar 10, 2025

do you use this pattern because the contents of the preamble closure need to run inside a ServiceGroup, for example because they use graceful shutdown?
I assume there's a reason this wouldn't work for you?

The database is also a service. So it needs to be running as well.

🤦‍♂️ I completely missed that databaseService, thanks.

And performDatabaseMigrations can't run as part of databaseService? The order of when databaseService and webserver come up is undefined, so I assume the way webserver uses the database already must have a way to wait for it to start processing requests?

@adam-fowler
Copy link
Member Author

🤦‍♂️ I completely missed that databaseService, thanks.

That's because I edited it

And performDatabaseMigrations can't run as part of databaseService? The order of when databaseService and webserver come up is undefined, so I assume the way webserver uses the database already must have a way to wait for it to start processing requests?

The way I see it databaseService comes from a library eg PostgresClient from PostgresNIO and the migrations are specific to the application, so they are two separate things

@czechboy0
Copy link
Collaborator

If the database should never be used until migrations have run, an alternative spelling would be for the Service implementation wrapping PostgresClient to offer a "postamble" closure, executed once the client is connected, but before it starts processing incoming requests. But if the database is meant to be used even before migrations, then this suggestion wouldn't work.

@adam-fowler
Copy link
Member Author

If the database should never be used until migrations have run, an alternative spelling would be for the Service implementation wrapping PostgresClient to offer a "postamble" closure

This wouldn't work as all Services start at the same time.

@czechboy0
Copy link
Collaborator

If the database should never be used until migrations have run, an alternative spelling would be for the Service implementation wrapping PostgresClient to offer a "postamble" closure

This wouldn't work as all Services start at the same time.

Right, them starting at the same time is why I suggested moving the "postamble" closure into the PostgresClient (in the wrapper might not be enough). It'd need some way to keep the client marked as not-yet-running (to avoid processing incoming requests yet), but also provide a handle to make requests only to the "postamble" closure, to be able to run startup steps without racing with other requests. But maybe that's unnecessarily complicated.

@FranzBusch
Copy link
Contributor

In general, I think we have three approaches for the problem at hand:

  • PreambleService: This is your PR here and we would nest ServiceGroups into each other to make sure some stuff ran before the nested service or service group starts
  • Introduce a StatefulService protocol that services can conform to so that you can say only start the next service after a certain state of the previous has been hit
  • wit-style graceful shutdown: In the end, both of the above are actually solved in structured concurrency however our run based approach leads us down to reinvent linear code flow.

I understand the pragmatism behind your proposed PR here but I'm not sure this is the overall approach that we should recommend. I would like us to think through the other design spaces as well before adding the PreambleService here. As you said this is something that's completely solvable in user space right now so not blocking anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants