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

Release method #1179

Open
saturn4er opened this issue Mar 20, 2024 · 10 comments
Open

Release method #1179

saturn4er opened this issue Mar 20, 2024 · 10 comments

Comments

@saturn4er
Copy link

Is your feature request related to a problem? Please describe.
Currently, the fx.App lifecycle management allows us to create and start application components seamlessly. However, there's a missing piece in the lifecycle: a clean, explicit way to release resources such as database connections or open files, which are initiated during provider invocation. This becomes particularly necessary in scenarios where the application might not reach the .Start phase, or when only a single job is needed, thus not requiring the full application start-stop cycle. Without an explicit release mechanism, we are forced to implement ad-hoc, potentially error-prone solutions to ensure resources are properly released.

Describe the solution you'd like
I propose the addition of a .Release method to the fx.App structure, alongside a new lifecycle hook: fx.OnRelease. This would allow developers to define clean-up functions that are executed when .Release is called, ensuring that all resources are properly closed or released, regardless of whether the application fully starts or not. This feature could be implemented as follows:

fx.Provide(fx.Annotate(fx.NewMySQLDB, fx.OnRelease(CloseMySQLDB)))

Here, CloseMySQLDB would be a function designed to cleanly shut down the MySQL database connection established by NewMySQLDB, and would be automatically invoked when fx.App.Release is called.

app := fx.New(...)
defer app.Release()

Describe alternatives you've considered
One alternative might be to manually manage resource release outside of the fx lifecycle, by storing references to all resources that need to be released and explicitly calling their release methods. However, this approach can quickly become cumbersome and error-prone, especially in larger applications with many resources to manage.

Another alternative could be to use the existing OnStop hook for releasing resources. However, this is not always viable because OnStop is only called if the application successfully starts, which may not always be the case, especially in applications that perform tasks without entering the full start-stop cycle.

Is this a breaking change?
This proposal should not introduce breaking changes to the existing fx API. The addition of fx.App.Release and fx.OnRelease would be backward compatible, offering additional functionality without altering the current behavior of existing applications using the fx library.

@sywhang
Copy link
Contributor

sywhang commented Mar 20, 2024

Thanks for submitting this issue. I want to clarify a few things here.

Another alternative could be to use the existing OnStop hook for releasing resources. However, this is not always viable because OnStop is only called if the application successfully starts, which may not always be the case, especially in applications that perform tasks without entering the full start-stop cycle.

OnStop hook is actually called even if the app fails to start because one of the start hooks fail. In fact, if an app fails to start, only the OnStop hooks whose OnStart counterparts successfully ran would be run, in reverse order of the OnStart hooks were run.

This is indeed for cleaning up any resources that may have been taken during OnStart execution phase.

If this was not clear from the existing documentation, we should go and address that, but I believe Fx already provides what you are looking for.

@saturn4er
Copy link
Author

Hi @sywhang,

The concern is more about application lifecycle.

  • fx.New -> dependency injection
  • fx.Start -> allocate some resources
  • fx.Stop -> release resources that were allocated in fx.Start

However, in the real world, dependency injection also includes resource allocation. And some applications don't even need to call fx.Start, but they still need to release resources that were allocated during dependency injection.

@JacobOaks
Copy link
Contributor

Hey @saturn4er - here's my two cents:

And some applications don't even need to call fx.Start, but they still need to release resources that were allocated during dependency injection.

From my understanding, these two statements are conflicting. If you have some resources you need to allocate and then release, that is a reason why a service does need to start/stop the app. In general, anything that is required to be cleaned up is supposed to be delegated to hooks, not run within constructors. Do you have any reasoning/examples behind avoiding starting/stopping the app?

@saturn4er
Copy link
Author

Hey @JacobOaks

If you have some resources you need to allocate and then release, that is a reason why a service does need to start/stop the app

Not really

For example, you have some function which builds set of options for your application, where you define providers for sql.DB, some repository and some service. And there's two binaries where you want to use it. For example in server and in some data migration command which populates db. In second one you don't need to start application, you can just provide Invoke which will accept service, call method of this service and return error.
fx_options.go

func applicationOptions() fx.Option {
  return fx.Options(
     NewDB,
     NewRepo,
     NewService,
     ...
  )
}

server.go

func main(){
  fx.New(
    applicationOptions(),
    fx.Provide(NewHTTPServer),
    fx.Invoke(RegisterStartServerHooks),
  ).Run()
}

data_migration.go

func main(){
   app :=  fx.New(
    applicationOptions(),
    fx.Invoke(MigrateDataUsingService),
  )
 if  err := app.Error(); err!=nil{
    panic(err)
 }
}

@saturn4er
Copy link
Author

@sywhang It seems like documentation label do not match this issue

@JacobOaks
Copy link
Contributor

JacobOaks commented Apr 2, 2024

Hi @saturn4er,

I think there's some core misunderstanding here. In general, an Fx app has two distinct phases. The first phase, initialization, is purely for initializing dependencies. Namely, it should not be used for starting anything that must be stopped later. That should happen in the second phase, execution. This phase runs any hooks registered during initialization, making sure to run associated stop hooks later. I would recommend reading these docs to become more familiar: https://uber-go.github.io/fx/lifecycle.html.

If you have a specific example that current lifecycle & module structure does not support, feel free to leave it here, but with the example you gave alone, it's not clear why you cannot structure your modules such that core functionality can be re-used by both programs while removing program-specific functionality from the common module.

@saturn4er
Copy link
Author

saturn4er commented Apr 2, 2024

Hi @JacobOaks, I understand the lifecycle of fx applications. The second example is not a long running applications it just runs some small job and dies. I'll provide more detailed example:
fx_options.go - common options

func applicationOptions() fx.Option {
  return fx.Options(
     fx.Provide(fx.Annotate(NewDB, fx.OnStop(CloseDB))), // Here we create database connection that should be closed. 
  )
}

server.go - this one is main service(application that starts and wait until linux will send some signal)
CloseDB will be executed as we call .Run

func main(){
	fx.New(
		applicationOptions(),
		fx.Provide(func(db *sql.DB) *http.Server {
			return &http.Server{
				Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
					// query database and write response	
				}),
			}
		}),
		fx.Invoke(func(lc fx.Lifecycle, server *http.Server) {
			lc.Append(fx.Hook{
				OnStart: func(ctx context.Context) error {
					return server.ListenAndServe()
				},
				OnStop: func(ctx context.Context) error {
					return server.Shutdown(ctx)
				},
			})
		}),
		).Run()
}

job.go - this one starts and dies after all invokes will be executed.
OnStop hook CloseDB will not be called as there's no .Run method call

func main(){
	if err := fx.New(
		applicationOptions(),
		fx.Provide(func(db *sql.DB) *http.Server {
			return &http.Server{
				Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
					// query database and write response	
				}),
			}
		}),
		fx.Invoke(func(db *sql.DB) error {
			// do some work with db, for example applying migrations and die
			return nil
		}),
	).Err(); err!=nil { 
		fmt.Println("err")
	}
}

@JacobOaks
Copy link
Contributor

JacobOaks commented Apr 2, 2024

Why can't you .Start() and .Stop() the app in job.go?

  fx.Invoke(func(db *sql.DB) error {
  	// do some work with db, for example applying migrations and die
  	return nil
  }),

FWIW, based on the comment, this invoke should probably also be in a start hook, and so should whatever initiates your connection to the DB (whatever needs cleaned up in CloseDB).

@saturn4er
Copy link
Author

saturn4er commented Apr 2, 2024

fx.Invoke(func(db *sql.DB, lc fx.Lifecycle, shutdowner fx.Shutdowner) {
	lc.Append(fx.Hook{
		OnStart: func(ctx context.Context) error {
			// do some work with db, for example applying migrations and die
			shutdowner.Shutdown()
			return nil
		},
	})
})

It looks like some workaround other than good solution. I don't need to start any application here, just invoke some function

@abhinav
Copy link
Collaborator

abhinav commented Apr 3, 2024

fx.Invoke(func(db *sql.DB, lc fx.Lifecycle, shutdowner fx.Shutdowner) {
	lc.Append(fx.Hook{
		OnStart: func(ctx context.Context) error {
			// do some work with db, for example applying migrations and die
			shutdowner.Shutdown()
			return nil
		},
	})
})

It looks like some workaround other than good solution. I don't need to start any application here, just invoke some function

FWIW, this snippet matches what I've been doing for short-lived Fx applications.
fx.New(...).Run() with that inside for the main function of the short-lived application seems to work as expected.

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

No branches or pull requests

4 participants