Fix the graceful shutdown

[2403905]

Added the reva HTTP/GRPC servers constructor w/o the watcher and the os signal handling to make it driven by the ocis runtime.

owncloud/ocis#11170

[jvillafanez]

Some things I've noticed:

• Logging a Fatal error and not returning immediately seems wrong. If the error is really fatal, there shouldn't be anything else for us to do, so either the error isn't fatal or there is a bug there.
• For the "drivenserver.go" file, I think it's better to split it into multiple files. There is a constant switching between drivenHTTPServer and drivenGRPCServer that makes it annoying to go through the file.
• We might skip the isEnabledHTTP and isEnabledGRPC checks. We can return a server instance (or nil) and an error instead, which is a common pattern. Then we can split the RunDrivenServerWithOptions into RunDrivenHTTPServerWithOptions and RunDrivenGRPCServerWithOptions so we can also return the corresponding instance with the error there.
  • To skip the isEnabledHTTP check, we can assume that oCIS will take care of it in some way because it's oCIS the one preparing the configuration, so it knows whether the HTTP or GRPC server will be needed and the required configuration should be there. As far as I know, reva only checks there is configuration for the service, but it doesn't check any value.

[2403905]

Logging a Fatal error and not returning immediately seems wrong. If the error is really fatal, there shouldn't be anything else for us to do, so either the error isn't fatal or there is a bug there.

Changed: After logging Fatal and returning nil or err, no matter what exactly the ocis anyway must die. The os.Exit(1) was replaced by Fatal because the os.Exit(1) kills the ocis earlier than all log messages printed.

We might skip the isEnabledHTTP and isEnabledGRPC checks. We can return a server instance (or nil) and an error instead, which is a common pattern. Then we can split the RunDrivenServerWithOptions into RunDrivenHTTPServerWithOptions and RunDrivenGRPCServerWithOptions so we can also return the corresponding instance with the error there.
To skip the isEnabledHTTP check, we can assume that oCIS will take care of it in some way because it's oCIS the one preparing the configuration, so it knows whether the HTTP or GRPC server will be needed and the required configuration should be there. As far as I know, reva only checks there is configuration for the service, but it doesn't check any value.

The constructors return nil if no server is expected or die if misconfigured.

[2403905]

@jvillafanez I assumed that we should stop the Listen first and then clean the services. Could you check if it makes sense?

[jvillafanez]

As far as I know, this Stop method will be call by a secondary thread because the main one will be running the service. Once we call s.s.Stop(), the main thread will keep going, so there is a risk that the app finishes without having run the s.cleanupServices() (or maybe it runs partially for some services until the app closes).
Maybe I'm wrong, there are too many Stop methods and it's hard to know which one we're calling here.

[kobergj]

Just small question. Code looks good 👍

[jvillafanez]

Just saying that throwing isn't logging.

The code is fine if we consider that the fatal log won't happen in practice. If something is wrong, it should be fixed during development.

[2403905]

Just saying that throwing isn't logging.

Why? I can rephrase it, the meaning remains the same. Both log.Fatal() and options.Logger.Fatal() will print a message and then call an os.Exit(1).
I can change // Throws a fatal error if the http server cannot be created. to // Call a fatal error if the http server cannot be created.

The only way wrong is if we call the logger and then exit(1) because there is a very big chance that we never see this log.

			options.Logger.Error().Err(err).Msg("error runnig server")
			w.Exit(1)

[jvillafanez]

It's just a wording change in the comment. Logs a fatal error seems more clear to me.
Throw is a keyword in other languages such as Java and PHP, and it have a very specific meaning. Even if we consider Go language, it might be confusing specially when the function doesn't return (or "throw") an error that we could handle upwards.
As said, the code is fine, it's just the wording.

[2403905]

ok. updated.

[jvillafanez]

I assume we don't want to be too strict with the gracefulShutdownTimeout waiting time. I mean, we spawn a goroutine to stop and wait, but there is no guarantee that the goroutine will have enough time to run. There is a chance that the we reach the timeout but the goroutine didn't even start to run.
Of course, this scenario is too extreme, even more the longer the timeout is. I'm fine with the current code for the sake of simplicity, but maybe we should document somewhere that the timeout shouldn't be less then 5 secs (or whatever value is the standard in our case)

[jvillafanez]

The "exit with code 1" is misleading because the function never does it. As it is written, it seems there is a os.Exit(-1) call hidden somewhere inside this function, but it isn't the case.

If, as a result of this function returning nil, the main app decides to exit with code 1, that's fine, but it's something this function doesn't need to care about. Whatever the caller does with the result of this function isn't this function's business.

[2403905]

I assume we don't want to be too strict with the gracefulShutdownTimeout waiting time. I mean, we spawn a goroutine to stop and wait, but there is no guarantee that the goroutine will have enough time to run. There is a chance that the we reach the timeout but the goroutine didn't even start to run. Of course, this scenario is too extreme, even more the longer the timeout is. I'm fine with the current code for the sake of simplicity, but maybe we should document somewhere that the timeout shouldn't be less then 5 secs (or whatever value is the standard in our case)

Added the comments.