storage: fix an error message when trying to backup to local drive on…

[darinpp]

… Windows

An often occuring pattern is:

{
...
f, _ := os.Create(someFile)
defer f.Close()
...rename or delete the file...
}

This does not work on Windows by default. It can be made to work (kindof) when
the file is opened with a special flag but the behaviour is not the same as
on a posix system.
There seems to be an effort to fix this in go as seen in issue https://github.com/golang/go/issues/32088
Also the default for the newer Windows versions seems to be to allow this https://github.com/golang/go/issues/32088#issuecomment-502850674
Until then - we should do an extra close before remove or delete (assuming these are the last operations).

Fixes #41085

Release justification: bug fix for existing functionality

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @darinpp, and @dt)

---

pkg/ccl/storageccl/export_storage.go, line 345 at r1 (raw file):

		return errors.Wrapf(err, "syncing to local export tmp file %q", tmpP)
	}
	f.Close()

If io.Copy or f.Sync() return an error we'll leak this open file. Another alternative is to move the rename into a defer. You'd have to give name the error return value. Something like:

func (l *localFileStorage) WriteFile(...) (err error) {

  f, err := os.Create(tmpP)
  ...
  defer func() {
    f.Close()
    if err == nil {
      err = errors.Wrapf(os.Rename(tmpP, p), "renaming to local export file %q", p)
    }
  }()

  // Copy, Sync, etc.
}

[dt]

I'd also be fine with just expanding the defer by hand, putting f.Close() before each of the two error returns, given the small number of early returns.

That said, given the numerous existing caveats with nodelocal, I'm also okay with it just not working on Windows for now, since I'm also wary of introducing any potential leaked FDs or other complexity this late in the release.

[darinpp]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @darinpp, and @dt)

pkg/ccl/storageccl/export_storage.go, line 345 at r1 (raw file):

		return errors.Wrapf(err, "syncing to local export tmp file %q", tmpP)
	}
	f.Close()

If io.Copy or f.Sync() return an error we'll leak this open file. Another alternative is to move the rename into a defer. You'd have to give name the error return value. Something like:

func (l *localFileStorage) WriteFile(...) (err error) {

  f, err := os.Create(tmpP)
  ...
  defer func() {
    f.Close()
    if err == nil {
      err = errors.Wrapf(os.Rename(tmpP, p), "renaming to local export file %q", p)
    }
  }()

  // Copy, Sync, etc.
}

This is a good point but deferring the rename doesn't allow us to return rename errors. So I simply added an extra defer f.Close() as there is no harm to always do an extra Close .

[darinpp]

I'd also be fine with just expanding the defer by hand, putting f.Close() before each of the two error returns, given the small number of early returns.

That said, given the numerous existing caveats with nodelocal, I'm also okay with it just not working on Windows for now, since I'm also wary of introducing any potential leaked FDs or other complexity this late in the release.

I changed it so we will now have always the deferred Close. So the new code is just an addition to the existing code and simply closes the file before rename.

[dt]

is it okay to double-close?

[darinpp]

is it okay to double-close?

Yes. You will get an error file already closed the second time that will be ignored for the deferred close.

[petermattis]

Yes. You will get an error file already closed the second time that will be ignored for the deferred close.

Is that true on Unix? close(2); close(2) might actually succeed if another thread happened to open file descriptor 2 in the interim. Note that you can change the err return value from the defer with my suggestion:

defer func() {
  if err == nil {
    err = <rename>
  }
}

[dt]

I'd find it a tad easier to follow if we just got rid of the defer and called close in the two early returns and then where we actually want to close the file / 2¢

[darinpp]

Yes. You will get an error file already closed the second time that will be ignored for the deferred close.

Is that true on Unix? close(2); close(2) might actually succeed if another thread happened to open file descriptor 2 in the interim. Note that you can change the err return value from the defer with my suggestion:

defer func() {
  if err == nil {
    err = <rename>
  }
}

Ok. I changed it back to do the rename in the defer.
The problem with closing twice doesn't exist as the second close is not just blindly passed to the OS. The error message is generated by GO and the second call is not allowed to result in a syscall,
But I can imagine some platform where this is not the case and your concern is valid.

[petermattis]

Thanks for checking further into the double-close. This structure makes me feel safer.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz and @petermattis)

[darinpp]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[darinpp]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[darinpp]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)