Don't open a new file descriptor for a locked state. #17636
Conversation
Refreshing a locked state on windows could return nil if the read path was locked, no state was yet written, and the read path is the same as the write path. Add a test that locks then refreshes a newly initialized state struct.
ReadState would hide any errors, assuming that it was an empty state. This can mask errors on Windows, where the OS enforces read locks on the state file.
Only open a new file descriptor for RefreshState if we haven't written a state, and don't have the correct state open already. This prevents windows from failing to refresh a locked state.
| // the error is either io.EOF or "invalid argument", and both are from | ||
| // an empty state. | ||
| return nil, ErrNoState | ||
| if err == io.EOF { |
There was a problem hiding this comment.
The removed comment here talks about both io.EOF and "invalid argument". Is the "invalid argument" case taken care of by the nil check above, or taken care of some other way here?
There was a problem hiding this comment.
oh yes, "invalid argument" is only expected when we have a nil *os.File. At one point that was ok to use for ReadState to indicate the absence of state. I'd like to not have that at all, but that takes some more serious auditing to make sure we don't have any more code paths passing it in.
|
FTR, All state tests passing when run directly on a Windows host. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Only open a new file descriptor for RefreshState if we haven't written a
state, and don't have the correct state open already. This prevents
windows from failing to refresh a locked state.
This also surfaces initial state read errors, which were previously assumed to be because of an empty state. On windows, this could mask the error return when file access is denied.
Fixes #17620