Fix for #379, save() ignores write failures #380
Conversation
| @@ -878,11 +880,25 @@ template<typename S, typename T, typename Distance, typename Random> | |||
| if (f == NULL) | |||
| return false; | |||
|
|
|||
| fwrite(_nodes, _s, _n_nodes, f); | |||
| fclose(f); | |||
| bool write_failed = false; | |||
There was a problem hiding this comment.
don't think this one is really needed, could just return false at each point?
not blocking though
There was a problem hiding this comment.
I refactored that a few times deciding if I wanted to unload or not in case of error. That is a vestige of that.
|
thanks! |
| t.add_item(i, v) | ||
| t.build(10) | ||
|
|
||
| if sys.platform == "linux" or sys.platform == "linux2": |
There was a problem hiding this comment.
| t.save("/dev/full") | ||
| self.fail("didn't get expected exception") | ||
| except Exception as e: | ||
| self.assertTrue(str(e).find("No space left on device") > 0) |
There was a problem hiding this comment.
Python nit: could do self.assertTrue("No space left on device" in str(e)) or in newer versions, self.assertIn("No space left on device", str(e))
| t.save('/Volumes/%s/annoy.tree' % volume) | ||
| self.fail("didn't get expected exception") | ||
| except Exception as e: | ||
| self.assertTrue(str(e).find("No space left on device") > 0) |
| elif sys.platform == "darwin": | ||
| volume = "FULLDISK" | ||
| device = os.popen('hdiutil attach -nomount ram://64').read() | ||
| os.popen('diskutil erasevolume MS-DOS %s %s' % (volume, device)) |
There was a problem hiding this comment.
This works, but I'm pretty wary of calling diskutil erasevolume from a unit test. Is there a simpler way we can cause an error? (Maybe writing to a special file, like a named pipe or socket instead?)
There was a problem hiding this comment.
Not sure a pipe would work. Might just block rather than out of disk space. I agree it is a bit wonky. Possibly we just test this on linux? Where it is clear and concise?
|
Great timing @erikbern! 😂 |
|
@erikbern When do you expect you're going to push out a new version? This fix is a for a production incident I'd like to get sorted out. Thanks! |
|
I can do that later tonight, but please remind me if I forget! |
|
The test doesn't seem to be working on OS X: |
|
Interesting, I wonder if one of the previous steps failed, and then didn't actually create the volume. Given that there is the linux test, I think you can just delete the mac test. |
|
ok, will just delete it |
|
@erikbern Any chance of getting out a new version this week so I can update our internals here? Keith |
|
sure can do tonight |
|
done |
No description provided.