[PATCH] fsmonitor: be robust in the face of bad state

[Martijn Pieters]

On 25 November 2016 at 15:30, Simon Farnsworth <simonfar at fb.com> wrote:

> # HG changeset patch
> # User Simon Farnsworth <simonfar at fb.com>
> # Date 1480087846 28800
> #      Fri Nov 25 07:30:46 2016 -0800
> # Node ID 0ca34f1b83da754246ee33e01c4f7d6652061f5d
> # Parent  a3163433647108b7bec8fc45896db1c20b18ab21
> fsmonitor: be robust in the face of bad state
>
> fsmonitor could write out bad state if interrupted part way through, and
> would then crash when it tried to read it back in.
>
> Make both sides of the operation more robust - reading state should fail
> cleanly, and we can use atomictemp to write out cleanly as the file is
> small. Between the two, we shouldn't crash with an IndexError any more.
>

I stepped through this with Simon; specifically using the atomic file
object as a context manager ensures that it never replaces the old state
file if an exception occurs; the previous version would close the new file
no matter what happened (and so leaving an incomplete file).

I traced what would happen if there are exactly 3 elements (so notefiles
would be an empty list); the code that uses state.get() can set notefiles
to an empty list in various places *anyway*, so the list being empty is a
valid edgecase.

In other words, looks great to me.



> diff --git a/hgext/fsmonitor/state.py b/hgext/fsmonitor/state.py
> --- a/hgext/fsmonitor/state.py
> +++ b/hgext/fsmonitor/state.py
> @@ -59,6 +59,12 @@
>              state = file.read().split('\0')
>              # state = hostname\0clock\0ignorehash\0 + list of files, each
>              # followed by a \0
> +            if len(state) < 3:
> +                self._ui.log(
> +                    'fsmonitor', 'fsmonitor: state file truncated
> (expected '
> +                    '3 chunks, found %d), nuking state\n' % len(state))
> +                self.invalidate()
> +                return None, None, None
>              diskhostname = state[0]
>              hostname = socket.gethostname()
>              if diskhostname != hostname:
> @@ -85,12 +91,12 @@
>              return
>
>          try:
> -            file = self._opener('fsmonitor.state', 'wb')
> +            file = self._opener('fsmonitor.state', 'wb', atomictemp=True)
>          except (IOError, OSError):
>              self._ui.warn(_("warning: unable to write out fsmonitor
> state\n"))
>              return
>
> -        try:
> +        with file:
>              file.write(struct.pack(_versionformat, _version))
>              file.write(socket.gethostname() + '\0')
>              file.write(clock + '\0')
> @@ -98,8 +104,6 @@
>              if notefiles:
>                  file.write('\0'.join(notefiles))
>                  file.write('\0')
> -        finally:
> -            file.close()
>
>      def invalidate(self):
>          try:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>



-- 
Martijn Pieters
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161125/88048a7d/attachment.html>