General Shutdown Handler registration is missing about iris HOT 7 CLOSED

We know the problem here but RegisterOnShutdown (the method that host sub-package owns), or yours OnHostShutdown, idea was declined by @kataras for design matters, details below.

He said that if we start adding events or functions that are not belong to the main package then what's the reason of concept separation and the sub-packages?, therefore if a user wants to do more with a host then the host package should be used or call the app.NewHost and pass its listen function to an Run(iris.Raw(host.ListenAndServe)), there is an RegisterOnInterrupt that is accessible on the main package.

However there is a middle solution that we have discussed already, the idea to pass shutdown events via Configurator, again we will have to keep track of shutdown events on the Application instance and register them when a host is created...if we start with this addition, after a month we may notice that a user needs more functions that belongs to host sub-package, then we'll have to add that function to the main instance as well?

Do you have any solutions in mind that we can use without destroying the current abstraction? Thanks!

from iris.

However there is a middle solution that we have discussed already, the idea to pass shutdown events via Configurator, again we will have to keep track of shutdown events on the Application instance and register them when a host is created...

That is what the PR proposes by doing that:

func TheConfigurator(app *iris.Application) {
    app.OnHostShutdown(func(h *host.Supervisor) {
        // Doing something
    })
}

func main() {
    app := iris.New()

    app.Configure(TheConfigurator)

    app.Run(iris.Addr(":8080"))
}

With the OnHostShutdown method, we keep track of shutdowns events on the Application instance and register them when a host is created. See the code of the PR line 369 to 379:

	makeHandler := func(handler func(supervisor *host.Supervisor)) func() {
		return func() {
			handler(su)
		}
	}

	// register shutdown handlers
	for _, handler := range app.shutdownHandlers {
		su.RegisterOnShutdown(makeHandler(handler))
	}

If you want i can rename the method AddShutdownHandler instead of OnHostShutdown.

from iris.

If you want i can rename the method AddShutdownHandler instead of OnHostShutdown.

There is a RegisterOnShutdown at https://github.com/kataras/iris/blob/master/core/host/supervisor.go#L254

// RegisterOnShutdown registers a function to call on Shutdown.
// This can be used to gracefully shutdown connections that have
// undergone NPN/ALPN protocol upgrade or that have been hijacked.
// This function should start protocol-specific graceful shutdown,
// but should not wait for shutdown to complete.
func (su *Supervisor) RegisterOnShutdown(cb func()) {
	// when go1.9: replace the following lines with su.Server.RegisterOnShutdown(f)
	su.mu.Lock()
	su.onShutdown = append(su.onShutdown, cb)
	su.mu.Unlock()
}

So I think it would be better if the name was RegisterOnShutdown on main package as well, for conversion.

That is what the PR proposes by doing that.. With the OnHostShutdown method, we keep track of shutdowns events on the Application instance and register them when a host is created. See the code of the PR line 369 to 379:

Yes, I read the PR and understood it, it's simple but with Configurator I meant built'n , something like iris.WithCharset, iris.WithoutStartupLog, etc... , so users could use a app.Run(iris.Addr(":8080"), iris.WithShutdown(shutdownHandler) or WithShutdownHandler, again this method is not based on the current implementation (Configuration takes fields that can be handled by toml and yaml files, strings and maps)...

My recommendation:

Change shutdownHandlers to onShutdown, as https://github.com/kataras/iris/blob/master/core/host/supervisor.go#L40 and OnHostShutdown to RegisterOnShutdown as https://github.com/kataras/iris/blob/master/core/host/supervisor.go#L254.

Also make sure(and rename to something more precise) that makeHandler is not duplicated (on app.NewHost and app.OnHostShutdown) by moving it outside of these functions.

I'll accept this, we will discuss later what we can do to reduce the code duplications that the PR has and the relation between the core/host package.

from iris.

The real problem is the modularity.

If i want to allocate resource in a middleware, i don't know when it must be cleaned. With your solution i must add this cleaning in main module, this way breaks the modularity.

A real example, is when you want to have the choice to activate or not the HTTP to HTTPS redirection. To have the choice, the design force us to put it in a middleware which is in another place (another package). We have to start the HTTP server in a middleware. It's not only true for this example but it's also true for all other resources allocated in a middleware.

Continue with a service started in a middleware for example to do a task every minute. First, let's start, with the middleware package, named here as the "counter" package:

package counter

import (
	sysContext "context"
	"time"

	"github.com/kataras/iris"
	"github.com/kataras/iris/context"
)

func CounterMiddleware(ctx sysContext.Context) iris.Configurator {
	counterValue := 0

	go func() {
		ticker := time.NewTicker()
		defer ticker.Stop()

		for {
			select {
			case <-ticker.C:
				counterValue++

			case <-ctx.Done(): // =============>> Shutdown
				return
			}
		}
	}()

	return func(app *iris.Application) {
		app.Get("/counter", func(ctx context.Context) {
			ctx.Header("Content-Type", "text/plain")
			ctx.Writef("Counter value = %d", counterValue)
		})
	}
}

And now the main package:

package main

import (
	sysContext "context"

	"counter"

	"github.com/kataras/iris"
	"github.com/kataras/iris/core/host"
)

func main() {
	app := iris.New()

	// The developer have to do that
	// but it would be avoided if hosts are accessible in middlewares
	ctx, cancel := sysContext.WithCancel(sysContext.Background())

	// Here again the developper must pass a parameter
	// to a function that constructs a Configurator
	// instead of pass a simple Configurator to app.Configure
	// without to instanciate it
	app.Configure(counter.CounterMiddleware(ctx))

	err := app.Run(iris.Addr(":8080", func(h *host.Supervisor) {
		h.RegisterOnShutdown(func() {
			cancel()
		})
	}))

	if err != nil {
		app.Logger().Error(err)
	}
}

Here the user of the middleware have to create a channel of communication to transport information of shutdown. It could be avoided. The middleware plugs on its own to the server and it shouldn't have the needing to have another cement to finish the wall.

The modularity is essential if you want publish some features. But you can see that this kind of feature cannot be published without additional interventions from developer, these interventions decays the wish to use it.

from iris.

@corebreaker I never released how iris.Configurator can be so useful for other developers, nice job!

We could just create a slice of func(h *host.Supervisor) and pass them to the app.Run,, that way an external module can inject the host without the need of accessing your Application.

However if we want to have access to the host in order to register a Shutdown handler from your iris.Configurator without developer's work then we have to implement a new simple function. That can be done fast, give me some time and you'll see how easy it was!

EDIT:
Done, upgrade with go get -u github.com/kataras/iris , the code lines are now being reduced and remember you have the full control of the host, not just the shutdown handlers.

package counter

import (
	"time"

	"github.com/kataras/iris"
	"github.com/kataras/iris/context"
	"github.com/kataras/iris/core/host"
)

func Configurator(app *iris.Application) {
	counterValue := 0

	go func() {
		ticker := time.NewTicker(time.Second)

		for range ticker.C {
			counterValue++
		}

		app.ConfigureHost(func(h *host.Supervisor) { // <- HERE: IMPORTANT
			h.RegisterOnShutdown(func() {
				ticker.Stop()
			})
		})
// or put the ticker outside of the gofunc and put the configurator
// before or after the app.Get, outside of this gofunc

	}()

	app.Get("/counter", func(ctx context.Context) {
		ctx.Header("Content-Type", "text/plain")
		ctx.Writef("Counter value = %d", counterValue)
	})
}

package main

import (
	"github.com/kataras/iris/_examples/http-listening/iris-configurator-and-host-configurator/counter"

	"github.com/kataras/iris"
)

func main() {
	app := iris.New()
	app.Configure(counter.Configurator)

       // no need to catch errors with app.Logger().Error, it does that automatically
      //  when level is not disabled         
      app.Run(iris.Addr(":8080")) 
}

Pretty simple, right?

from iris.

Okay, that sounds nice.
I'm going close the PR, i agree your solution.

from iris.

Thank you, you made the framework even better with your proposes and your ideas, keep that habit

from iris.