Write file atomically, watch create events, handle empty files. #56

sayrer · 2017-09-07T18:02:04Z

No description provided.

tdarci

lgtm

tdarci · 2017-09-07T18:39:00Z

cli/api/ioutil2/ioutil.go

@@ -0,0 +1,51 @@
+/*


perhaps we should include a mention here of where this file came from?

https://github.com/youtube/vitess/blob/master/go/ioutil2/ioutil.go

tdarci · 2017-09-07T18:40:34Z

cli/api/client.go

@@ -338,7 +338,7 @@ func (c *Client) WriteOutputFile(kvb stores.KVBytes) {
 		os.Exit(1)
 	}

-	err = ioutil.WriteFile(c.config.Watcher.OutputPath, bts, 0644)
+	err = ioutil2.WriteFileAtomic(c.config.Watcher.OutputPath, bts, 0644)


Do we know if non-Go clients depend on the modify-in-place behavior (and don't listen on Create events)?

I checked the node.js and PHP clients--they seem fine.

negator · 2017-09-07T18:49:54Z

cli/api/ioutil2/ioutil.go

+
+// Write file to temp and atomically move when everything else succeeds.
+func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error {
+	dir, name := path.Split(filename)


Dont we need to check if dir and name are valid? i.e. not empty?

n.b. this is code from https://github.com/youtube/vitess/blob/master/go/ioutil2/ioutil.go

I didn't want to import that whole project. That will fail below (and won't happen to us, I think)

negator · 2017-09-07T18:51:48Z

cli/api/ioutil2/ioutil.go

+	if err != nil {
+		return err
+	}
+	_, err = f.Write(data)


Don't we want to exit immediately on error (and cleanup temp file)? This code will execute every subsequent operation after an error.

The error handling is a little weird, but looks correct, and I don't want to rewrite this external file

vladlosev · 2017-09-07T20:21:55Z

client/watcher/watcher.go

@@ -74,8 +74,11 @@ func (w *Watcher) Watch() {
 			w.mu.Lock()
 			select {
 			case event := <-w.watcher.Events:
-				if event.Op&fsnotify.Write == fsnotify.Write {
-					w.UpdateBytes()
+				if event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create {


Is this code supposed to work exclusively with the file-writing code in cli/api/client or is it meant to be generic? In the latter case, it may still observe partial writes, of course. You can use the inotify methods in the unixpackage directly to listen for the IN_CLOSE_WRITE and the IN_MOVED_TO events in order to fix that. You also won't need atomic writes. And in the former case, the code does not need to handle the fsnotify.Write case.

It's supposed to be somewhere in between. If the producer is using ioutils.WriteFile, a partial write will signal an error. (https://golang.org/src/io/ioutil/ioutil.go?s=2722:2780#L76)

Doing it this way will preserve compatibility with users that have extended this package in some way (and help roll out our own upgrade...)

The atomic writes are necessary for the PHP client, I think. It seems to be occasionally picking up a truncated file from ioutils.WriteFile. The node client also responds to every event without checking. We could fix all these, or just switch to atomic writes. I think the latter is simpler for consumers.

melindalu · 2017-09-07T23:17:27Z

client/watcher/watcher.go

@@ -107,6 +110,10 @@ func (w *Watcher) UpdateBytes() error {
 		return err
 	}

+	if len(bts) == 0 {


melindalu · 2017-09-07T23:17:39Z

client/watcher/watcher.go

+				if event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create {
+					err := w.UpdateBytes()
+					if err != nil {
+						printer.LogErrf("[dcdr] watch error: %v", err)


nit: can we call this an updateBytes error or something to differentiate from L84?

melindalu · 2017-09-07T23:18:16Z

cli/api/client.go

@@ -338,7 +338,7 @@ func (c *Client) WriteOutputFile(kvb stores.KVBytes) {
 		os.Exit(1)
 	}

-	err = ioutil.WriteFile(c.config.Watcher.OutputPath, bts, 0644)
+	err = ioutil2.WriteFileAtomic(c.config.Watcher.OutputPath, bts, 0644)


Do we know if non-Go clients depend on the modify-in-place behavior (and don't listen on Create events)?

sayrer · 2017-09-08T01:26:35Z

(I got vlad's verbal ok for this)

sayrer added 6 commits September 7, 2017 11:01

Write file atomically, watch create events, handle empty files.

f447968

Import errors package.

fcb36d4

Fix typos

f180b85

Unused import

9d141b4

Merge

90af09c

Tests pass

33435c0

sayrer requested review from rquiz, promiseofcake, vladlosev and melindalu September 7, 2017 18:31

tdarci reviewed Sep 7, 2017

View reviewed changes

negator reviewed Sep 7, 2017

View reviewed changes

Add comment

fa10c33

vladlosev reviewed Sep 7, 2017

View reviewed changes

sayrer added 3 commits September 7, 2017 15:08

Change the way we track this event.

dff600f

Change the way we track this event.

6b52678

Use sync.Once correctly

10d68ef

rquiz approved these changes Sep 7, 2017

View reviewed changes

melindalu reviewed Sep 7, 2017

View reviewed changes

sayrer merged commit 5492539 into master Sep 8, 2017

sayrer deleted the atomic_file_write branch September 8, 2017 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write file atomically, watch create events, handle empty files. #56

Write file atomically, watch create events, handle empty files. #56

sayrer commented Sep 7, 2017

tdarci left a comment

tdarci Sep 7, 2017

sayrer Sep 7, 2017

tdarci Sep 7, 2017

melindalu Sep 7, 2017

sayrer Sep 7, 2017

negator Sep 7, 2017

sayrer Sep 7, 2017

negator Sep 7, 2017

sayrer Sep 7, 2017

vladlosev Sep 7, 2017 •

edited

Loading

sayrer Sep 7, 2017 •

edited

Loading

sayrer Sep 7, 2017 •

edited

Loading

melindalu Sep 7, 2017

melindalu Sep 7, 2017

melindalu Sep 7, 2017

sayrer commented Sep 8, 2017

Write file atomically, watch create events, handle empty files. #56

Write file atomically, watch create events, handle empty files. #56

Conversation

sayrer commented Sep 7, 2017

tdarci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladlosev Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

sayrer Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

sayrer Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayrer commented Sep 8, 2017

vladlosev Sep 7, 2017 •

edited

Loading

sayrer Sep 7, 2017 •

edited

Loading

sayrer Sep 7, 2017 •

edited

Loading