Codecov Report

❌ Patch coverage is 48.70130% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.95%. Comparing base (e78715c) to head (4bd410c).
⚠️ Report is 8 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1221      +/-   ##
==========================================
- Coverage   66.66%   65.95%   -0.71%     
==========================================
  Files         237      243       +6     
  Lines       15879    16676     +797     
==========================================
+ Hits        10585    10998     +413     
- Misses       4928     5298     +370     
- Partials      366      380      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hey @goravel/core-developers Could you help confirm this and another PRs? The initiation framework process is changed in these two PRs totally. We can have a discussion on this.

Amazing job👍 This change makes main.go much cleaner. Currently, in our framework, Route, Grpc, Queue, and Schedule are installed as facades on demand. Would it be possible to abstract the Run logic into an interface, where modules could be registered in a way similar to a Service Provider during installation? This approach could also accommodate potential third-party extensions that might require starting services in the main thread in the future.

@almas-x Good point to provide a way to support the third package. I'll think about it.

Added a new type Runner interface, PTAL @almas-x @kkumar-gcc .

Added a new type Runner interface, PTAL @almas-x @kkumar-gcc .

Most of these runners in foundation/runners.go are quite similar, and currently the Application.Run method hardcodes runner for route, grpc, queue, and schedule. Would it be make sense to move the Runner implementation into ServiceProvider? For example, we could define an interface like:

type ServiceProviderWithRunner interface {
    ServiceProvider
    Runner() Runner
}

After loading all ServiceProviders, we could check if any implement Runner, and start their runners if so. This would allow runners to be installed and registered automatically with their respective ServiceProviders.

@almas-x Good idea, only have one concern: how to stop a runner in this way? According to the current logic, users can customize the runners: Run(runners ...Runner).

@almas-x Good idea, only have one concern: how to stop a runner in this way? According to the current logic, users can customize the runners: Run(runners ...Runner).

I think the graceful shutdown logic could be moved into Application.Run. Start each runner one by one, and when a quit signal is received, gracefully shut down each runner in order. This centralizes lifecycle management and makes the codebase more maintainable. What are your thoughts on this? @hwbrzzl

@almas-x Do you mean:

@almas-x Good idea, only have one concern: how to stop a runner in this way? According to the current logic, users can customize the runners: Run(runners ...Runner).

@hwbrzzl I feel runner is more closely related to a facade rather than the service provider itself, since a single provider can expose multiple facades. So instead of making the provider implement runner logic, maybe the provider can just tell us which of its facades implement Runner or Shutdown, and we can handle those automatically.

Something like how we already define Relationship, maybe we can add fields for that:

func (r *ServiceProvider) Relationship() contractsbinding.Relationship {
    bindings := []string{
        contractsbinding.Orm,
        contractsbinding.DB,
        contractsbinding.Schema,
        contractsbinding.Seeder,
    }

    return contractsbinding.Relationship{
        Bindings:     bindings,
        Dependencies: binding.Dependencies(bindings...),
        ProvideFor:   []string{},
        Runner:       []string{},   // facades implementing Runner
        Shutdown:     []string{},   // facades implementing Shutdown
    }
}

This way, the framework can look up which facades actually implement those interfaces and handle start/stop automatically.

@almas-x Do you mean:

Yep 😉

@almas-x Good idea, only have one concern: how to stop a runner in this way? According to the current logic, users can customize the runners: Run(runners ...Runner).

@hwbrzzl I feel runner is more closely related to a facade rather than the service provider itself, since a single provider can expose multiple facades. So instead of making the provider implement runner logic, maybe the provider can just tell us which of its facades implement Runner or Shutdown, and we can handle those automatically.

Something like how we already define Relationship, maybe we can add fields for that:

func (r *ServiceProvider) Relationship() contractsbinding.Relationship {
    bindings := []string{
        contractsbinding.Orm,
        contractsbinding.DB,
        contractsbinding.Schema,
        contractsbinding.Seeder,
    }

    return contractsbinding.Relationship{
        Bindings:     bindings,
        Dependencies: binding.Dependencies(bindings...),
        ProvideFor:   []string{},
        Runner:       []string{},   // facades implementing Runner
        Shutdown:     []string{},   // facades implementing Shutdown
    }
}

This way, the framework can look up which facades actually implement those interfaces and handle start/stop automatically.

That’s a great point, I forgot that a single ServiceProvider can provide multiple facades. However, currently only built-in facades can be installed via artisan package:install for self-service installation, and third-party extensions are not yet supported. So maybe we can change it so that a ServiceProvider exposes multiple runners, for example:

type ServiceProviderWithRunners interface {
    ServiceProvider
    Runners() []Runner
}

@kkumar-gcc I feel it's a bit strange that Relationship() contains Runners. And my main concern is that a service provider is installed, but user doesn't want to run it. How to achieve this. We may set different conditions for different facades, e.g. for Route, http.host should not be empty, for Grpc, grpc.host should not be empty, etc. It's very complex for user to understand such logic.

In the current logic, if user doesn't pass any runner, we will judge the conditions above. But if user passes runners, the runners will be Run and Shutdown without internal conditions.

@kkumar-gcc I feel it's a bit strange that Relationship() contains Runners. And my main concern is that a service provider is installed, but user doesn't want to run it. How to achieve this. We may set different conditions for different facades, e.g. for Route, http.host should not be empty, for Grpc, grpc.host should not be empty, etc. It's very complex for user to understand such logic.

In the current logic, if user doesn't pass any runner, we will judge the conditions above. But if user passes runners, the runners will be Run and Shutdown without internal conditions.

These checks can be handled inside the implementation of ServiceProviderWithRunners.Runners, for example:

func (r *ServiceProvider) Runners() (runners []foundation.Runner) {
    if configFacade != nil && configFacade.GetString("http.default") != "" {
        runners = append(runners, &RouteRunner{})
    }
    return runners
}

This way, the logic for whether to run a specific runner can be encapsulated within the service provider itself.

@kkumar-gcc I feel it's a bit strange that Relationship() contains Runners. And my main concern is that a service provider is installed, but user doesn't want to run it. How to achieve this. We may set different conditions for different facades, e.g. for Route, http.host should not be empty, for Grpc, grpc.host should not be empty, etc. It's very complex for user to understand such logic.

In the current logic, if user doesn't pass any runner, we will judge the conditions above. But if user passes runners, the runners will be Run and Shutdown without internal conditions.

@hwbrzzl Hmm, I agree, it feels odd to put runners in Relationship(). As @almas-x suggested, we can add Runners() []Runner and let each runner's constructor handle any required arguments. If we want users to control which runners start, add an optional ShouldRun() method on the runner so the runner can decide whether to initialize by default or via config. We can also let Runners() return facade/binding names instead of runner instances.

type Runner interface {
    ShouldRun() bool
    Run() error
    Shutdown() error
}

type ServiceProvider interface {
    Runners() []Runner | []Bindings
}

Example:

type Example struct {
    host string
}

func (r *Example ) ShouldRun() bool {
    return r.host != "" // or any other condition derived from config facade
}

func (r *Example) Run() error {
    return nil
}

func (r *Example ) Shutdown() error {
    return nil
}

Updated, PTAL. @goravel/core-developers

Addressed comments, PTAL @goravel/core-developers