Don't crash on windows

[kohenkatz]

As discussed in #44, #40, and #21, it is currently impossible to build code on Windows that references tableflip, even if that code never actually runs tableflip.

This PR addresses that issue in two ways:

1. Allow the tableflip code to compile without errors on Windows. Code in fds.go that uses syscall is separated into conditionally-compiled files dup_fd.go and dup_fd_windows.go. This follows the existing pattern in dup_file.go and dup_file_legacy.go. Similarly, code in env.go which uses syscall is separated into conditionally-compiled files env_syscalls.go and env_windows.go. It is important to note that the code will not work even though it compiled successfully. That is why the next part is needed.
2. Allow Windows users to test/run code that was built using tableflip with minimal changes. Upgrader is modified to return a tableflip.ErrNotSupported error when running on Windows. Additionally, a new package github.com/cloudflare/tableflip/testing is provided that contains a simple stub implementation of the Upgrader and Fds classes. As shown in the example provided therein, the user can define an interface with the required methods, then check the result of tableflip.New to see if errors.Is(err, tableflip.ErrNotSupported. If that is the case, the user can create an instance of the stub and use that instead.

[kohenkatz]

The one open question I have is whether it is better to "opt-out" for Windows (as I have done) or to "opt-in" for known-supported Linux and macOS.

On one hand, I would assume that this works on the *BSD platforms, so "opt-in" would be a breaking change for them. On the other hand, it doesn't seem like anyone is complaining about any other platform (though the only Go platform as yet unmentioned that I know of is Solaris/illumos).

[sagikazarmark]

From an application point of view I would expect that this library will just transparently not work on windows, meaning the application should not have to handle windows specific branches (is it what ErrNotSupported is for?).

Not sure what the current implementation does, my 2 cents only.

[kohenkatz]

@sagikazarmark no, it is not handled transparently. The point of returning ErrNotSupported is to inform the application that there is no way it can work. It then becomes the responsibility of the application developer to handle that error. One of the possible ways that the developer can do that is by using the stub implementation provided here. However, the point is to allow the developer to choose what happens in the event of running on an unsupported system.

[lmb]

From an application point of view I would expect that this library will just transparently not work on windows, meaning the application should not have to handle windows specific branches (is it what ErrNotSupported is for?).

I don't really want to get into the business of defining what "transparently not work" means TBH. tableflip isn't cross platform, and your application will have to deal with that in some way. With @kohenkatz's changes, code completion and compiling will work out of the box. If you want to be able to also run your binary we've got a ready made stub for you. If you later decide that you want a Windows specific implementation of this, you can easily incorporate it by checking for ErrNotSupported. You can also come up with your own stub, that better suits your needs. Finally, we don't have to litter all public functions of Upgrader with calls to isSupportedOS.

[sagikazarmark]

Okay, let me rephrase it: how would an application look like that needs to run on Windows (for development purposes), but is primarily targeted at unix platforms?

Do I have to use a different instance on windows?

Finally, we don't have to litter all public functions of Upgrader with calls to isSupportedOS

That's kinda what platform build tags are for IMHO.

I agree that the behavior is not cross platform, and I think it's also fine if the final upgrade call exits with an error (the application is shutting down anyway), but listeners and everything else could just work the same way.

There is a chance I don't see the whole picture, but a not supported error and a separate stub all suggest that I will have to use a different instance based on whether I'm on windows or not which could already be decided build time by the library. I think it would even be okay to have a constructor function with different implementations on different platforms.

[kohenkatz]

how would an application look like that needs to run on Windows (for development purposes), but is primarily targeted at unix platforms?

The minimum required change is defining an interface and adding two lines of code:

Before:

var upg tableflip.Upgrader
upg, err := tableflip.New(tableflip.Options{
    PIDFile: *pidFile,
})
if err != nil {
    panic(err)
}

After:

type upgrader interface {
    Listen(network, addr string) (net.Listener, error)
    Stop()
    Upgrade() error
    Ready() error
    Exit() <-chan struct{}
}

and

var upg upgrader
upg, err := tableflip.New(tableflip.Options{
    PIDFile: *pidFile,
})
if errors.Is(err, tableflip.ErrNotSupported) {
    upg, _ = testing.New()
} else if err != nil {
    panic(err)
}

[lmb]

There is a chance I don't see the whole picture, but a not supported error and a separate stub all suggest that I will have to use a different instance based on whether I'm on windows or not which could already be decided build time by the library.

Yes, you'll have to define an interface and then use testing.Upgrader when encountering ErrNotSupported. Based on the example in this PR it's about 10 lines of code. I don't think that is too onerous. Do you?

I think it would even be okay to have a constructor function with different implementations on different platforms.

Then you as a user can't discern whether you're on a supported platform without trying to do an upgrade. Not great.

I see these benefits with the current approach:

1. Users that don't use or develop on Windows don't have to deal with Windows support
2. Users can discern whether they are on a supported platform
3. Based on this they can either stub out the library
4. Or use a different library that works on windows

Using build tags means 3 and 4 are not straightforward. As a bonus, the current approach is useful if you want to unit test your main function: in tests you can pass in testing.Upgrader or your own implementation to make sure your code behaves as expected.

[lmb]

Thanks!