On non-Windows OS, hold file lock while renaming WAL directory

[aaronlehmann]

Windows requires this lock to be released before the directory is
renamed. But on unix-like operating systems, releasing the lock and
trying to reacquire it immediately can be flaky if a process is forked
around the same time. The file descriptors are marked as close-on-exec
by the Go runtime, but there is a window between the fork and exec where
another process will be holding the lock.

[aaronlehmann]

Seems like this made TestOpenOnTornWrite flaky in CI, but I can't reproduce locally.

This change just restores the code that existed before 5991209 for non-Windows platforms. Any ideas?

[heyitsanthony]

Split these separate code paths into the files wal_windows.go and wal_unix.go with windows and !windows build constraints? Each file would provide a platform specific implementation of func (w *WAL) renameWal(tmpdirpath string) (*WAL, error) for Create to call.

[heyitsanthony]

fn := w.tail().Name() in TestOpenOnTornWrite is probably getting a path with the old tmpdirpath since the reopen is gone. Try fn := path.Join(p, path.Base(w.tail().Name()))

[aaronlehmann]

Updated with these changes, thanks.

I named the files wal_windows.go and wal_other.go so that the non-Windows file is not unix-specific, but I'm happy to change this if desired.

[heyitsanthony]

A suffix that's used by go is preferable...

find /usr/lib/go/src/ | grep '_' | grep -v test | cut -f2 -d'_' | sort | grep "\.go" | uniq -c | sort -n | tail
     14 freebsd.go
     14 posix.go
     15 nacl.go
     15 openbsd.go
     16 darwin.go
     19 solaris.go
     26 linux.go
     37 unix.go
     45 plan9.go
     51 windows.go

So, _posix.go?

[heyitsanthony]

add the contents of $REPO_ROOT/.header here or it won't pass CI

also keep the build constraint a line separate from the package declaration because it confuses godoc

<HEADER>

// +build !windows

package wal

[heyitsanthony]

Please fix the last few comments and change the commit title to something like "wal: hold file lock while renaming WAL directory on non-windows" so it fits the commit title format for CI. Should be good to merge after that. Thanks!

[aaronlehmann]

In the Go source, when there's a *_windows.go file, they seem to use _unix.go and _plan9.go as counterpart suffixes. For example src/syscall/env_{windows,unix,plan9}.go and src/os/path_{windows,unix,plan9}.go. So I guess wal_unix.go seems like the best choice after all. I'll change it to that name if that sounds good. I'll also make the other modifications you suggested.

[heyitsanthony]

Either _unix.go or _posix.go is fine. Thanks!

[aaronlehmann]

Updated the PR, thanks.

[heyitsanthony]

lgtm

[xiang90]

do we still need this build tag? the file name is already _unix.go

[aaronlehmann]

The *_unix.go files in the Go standard library all seem to have build tags, but the *_windows.go files do not.

[xiang90]

ok. thanks.

[xiang90]

lgtm

[xiang90]

The test failure is unrelated to this pr. Merging.