owncloud / owncloud/ocis

List of os.Exit calls that need to be checked

Open
#8,968 1 comment 0 reactions 0 assignees View on GitHub
Type:Bug
Dominant language
Go
Stars
2.1k
Forks
274
Avg merge
2d 1h
Merged PRs (30d)
103

Description

## Describe the bug
As of April 25th, github shows 85 files containing the `os.Exit` call. Some of them could be fine and others wrong. Since using the `os.Exit` call is delicate, I think we should follow some general usage rules. Any exception to the rules should be clearly documented
* `os.Exit` must be called only by the main goroutine.
Calls from other goroutines are always a problem because the main goroutine will never finish (which could cause issues due to an unclean exit), and also because we don't know if there are other goroutines running (which could cause operations to be left in an inconsistent state due to the abrupt exit).
* The main goroutine should call `os.Exit` only if there are no other goroutine running.
* The general approach is to exit only during startup errors, otherwise the main goroutine should wait for the other goroutines to finish. Not waiting could cause issues because of inconsistent states caused by the abrupt exit while running an operation in a goroutine.
* If the main goroutine needs to exit early, it should notify the goroutines to finish. Common practice is to use `context` to signal when the goroutines need to finish by using the context's `Done()` channel. There could be other mechanisms available.
* The only exception to this rule should be if a goroutine is stuck and unable to finish. In this case, the main goroutine can call the `os.Exit` function after a predefined timeout.
Basically, we've waited long enough for the goroutine, so we assume it's stuck somewhere.
* On success cases, the main goroutine should finish normally without calling `os.Exit`. As said, the main goroutine should wait for other goroutines to finish, so if the main goroutine is still running that means that other goroutines are also running and we're waiting for them.

As said, any exception to those rules should be clearly documented and isolated. I'd expect such code to be used internally, so we need to know its expected usage and why some regular code (without the exit) isn't used instead.

-----

Grouped list of files containing the `os.Exit` call:
* `ocis/pkg/runtime/README.md`
Not critical, but the example could set a wrong precedent.
The secondary goroutine (spawed with the `time.AfterFunc`) can use a context, a channel or a waitGroup to notify when it has finished.
* `*/main.go`
These are fine because they comply with the requirements above.
* `ocis/pkg/command/migrate.go`
It might be okish assuming the command runs in an isolated environment, or it's the only command that is running. It could be improved though:
* It should return an error instead of calling `os.Exit` during the data validation.
* For the goroutines, they could use a `chan error` to return either the error response or nil if they finish without errors.
* `ocis-pkg/config/configlog/log.go`
This needs some extra documentation
Although it's documented that the `ReturnFatal` calls `os.Exit`, the expected usage is missing. Why not using a regular logger instead of any of those functions? Or why using a regular logger when a can use those functions anywhere?
My first impression was to remove the file because the regular logger should be enough and also it should provide a common formatting. I assume there are reasons to have the package around, but they should be documented.
* `ocis/pkg/runtime/service/service.go`
The `trap` function will need changes. The main goroutine won't finish nicely and the HTTP server (which is where the main goroutine is waiting) will be killed. As said, this is very risky because there could be operations ongoing and we could end up in an inconsistent state.
* `*/command/server.go`
Any server / service calling `os.Exit` is bad because there could be other services running.
We have already plans ongoing with https://github.com/owncloud/ocis/pull/8802 that should help to solve this issue
* `docs/helpers/env-var-delta.go`
This seems to be doc-only so probably not so important. Anyway, returning an error and handling it is a better solution.
* `ocis-pkg/natsjsregistry/registry.go`
I think we'll need to decide what is the expected behavior in the affected scenarios. In any case, exiting isn't a good idea.
* `services/storage-users/pkg/command/uploads.go`
As with any other command, it's better to return an error instead of exiting the program.
* `services/idp|proxy/pkg/server/http/server.go`
Returning an error seems a better choice.
* `ocis-pkg/log/log.go`
Exiting just because we couldn't log seems too hard. Alternative options could be:
* If we can't log to a file -> log to stderr / ignore the file option. This could be a valid fallback without having to exit the program. In addition, we could log a "we couldn't open the log file" entry.
* Return an error, although we'll need to adjust the code in multiple places.

## Steps to reproduce
1.
2.
3.

## Expected behavior
Calls to the `os.Exit` function should be limited, as described above

## Actual behavior
Some of the calls could cause issues by leaving data in an inconsistent state.

## Setup
Most of the issues could happen in the all-in-one setup because if one service is down, the whole system could die. Having separated services might mitigate some of the problems.

```console
OCIS_XXX=somevalue
OCIS_YYY=somevalue
PROXY_XXX=somevalue
```

## Additional context
Add any other context about the problem here.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.