Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Don't defer Close() on writable files (2017)

joeshaw.org

Post reply on HN
Print view View original

211–220 of 311 posts

Re: Don't defer Close() on writable files (2017)

#211

Post by cryptonector »

You don't want to just `fsync()`, but also flush whatever is buffered and then `fsync()`.

Another thing is that if you're holding an flock on that file, it's nice that closing it will drop the lock. Generally you want to drop the lock as soon as you're done writing to the file. Deferring the dropping of the lock to the deferred close might cause the lock to be held longer than needed and hold back other processes/threads -- this doesn't really apply in TFA's example since you're returning when done writing, but in other cases it could be a problem.

Do not risk closing twice if the interface does not specifically allow it! Doing so is a real good way to create hard-to-find corruption bugs. In general if you see `EBADF` when closing any fd values other than `-1`, that's a really good clue that there is a serious bug in that code.

Re: Don't defer Close() on writable files (2017)

#212

Post by cryptonector »

account42 wrote: post #194
paulsutter wrote: post #190

Earlier quoted context omitted.

Sqlite is much better than raw files to keep data intact on power failures, and be sure to study the options carefully If you truly need to use files you can take other steps such as mv the old file to .bck before mv the new file, but I really think you want sqlite

SQLite doesn't do any magic other than fsync. Using it to deal with power failures is nonsense.

`fsync()` is not necessarily enough if your on-disk format is complex because you still need to write recovery code. Of course, SQLite3's on-disk format is very complex, and so it requires correspondingly more complex power failure recovery code. But SQLite3 has an excellent test suite, and in particular they have an excellent power failure recovery test suite.

Using SQLite3 to avoid power failure issues is pretty good advice. I don't see why GP is getting downvoted.

Re: Don't defer Close() on writable files (2017)

#213

Post by o11c »

kccqzy wrote: post #152
dist-epoch wrote: post #13

Earlier quoted context omitted.

So if I do fopen/fwrite/fsync/fclose, that is not enough? That is crazy, I think 90% of apps don't fsync the parent directory. Also, how many levels of parents do you need to fsync?

You don't need even to bother with fsync unless you are developing a database or use the file system like a database. There's a reason Apple made fsync useless and introduced F_FULLSYNC. They know developers have an incentive to overestimate the importance of their own files at the detriment to system responsiveness and power draw.

The problem is, Apple's fsync doesn't even introduce any useful semantics. All it says is "write this eventually".

What a lot of people really need is just a "provide ordering, don't leave me with inconsistent data" operation. If you lose power "after" a write but before the fsync, for any non-networked application that's no different than losing power before the write, as long as the filesystem doesn't introduce chaos.

Re: Don't defer Close() on writable files (2017)

#214

Post by cryptonector »

russdill wrote: post #207

On production systems, it may often be better to completely ignore the problem at this level. On modern hardware if a disk is throwing an io error on write you are having a bad day. I can almost guarantee that while you might happily modify your code so it properly returns the error, there almost certainly aren't test cases for ensuring such situations are handled "correctly", especially since the error will almost c…

Please no. Just handle errors from `close()` when you're writing files.

> On modern hardware if a disk is throwing an io error on write you are having a bad day.

And how would you know you're having a bad day if apps ignore those errors?

Re: Don't defer Close() on writable files (2017)

#215

Post by cryptonector »

dist-epoch wrote: post #13
masklinn wrote: post #7

There are more wrinkles with this: - if you are creating a file, to ensure full synchronisation you also need to fsync the parent directory, otherwise the file can be fsynced but the update to the directory lost - if sync fails, you can not assume anything about the file, whether on-disk or in memory , critically one understanding which got dubbed "fsyncgate" and lead to many RDBMS having to be updated is that you ca…

So if I do fopen/fwrite/fsync/fclose, that is not enough? That is crazy, I think 90% of apps don't fsync the parent directory. Also, how many levels of parents do you need to fsync?

You need to `fflush()` too, before the `fsync()`.

Re: Don't defer Close() on writable files (2017)

#216

Post by surajrmal »

nextaccountic wrote: post #188

This is also why, in Rust, relying on drop to close a file (which ironically is the poster child for RAII) is a bad pattern. Closing a file can raise errors but you can't reasonably treat errors on drop. What we really need is a way to handle effects in drop; one way to achieve that is to have the option to return Result in a drop, and if you do this then you need to handle errors at every point you drop such a varia…

I actually really like this idea. Is there somewhere I can read more about it?

Re: Don't defer Close() on writable files (2017)

#217

Post by russdill »

cryptonector wrote: post #214
russdill wrote: post #207

On production systems, it may often be better to completely ignore the problem at this level. On modern hardware if a disk is throwing an io error on write you are having a bad day. I can almost guarantee that while you might happily modify your code so it properly returns the error, there almost certainly aren't test cases for ensuring such situations are handled "correctly", especially since the error will almost c…

Please no. Just handle errors from `close()` when you're writing files. > On modern hardware if a disk is throwing an io error on write you are having a bad day. And how would you know you're having a bad day if apps ignore those errors?

I'm arguing that the proper thing to do here is to kill the process along with whatever else is using the block device.

Whether you handle the error or immediately or if you allow the error to occur after a defer, you still are almost certainly not handling it properly and are taking a speed hit for your troubles.

Re: Don't defer Close() on writable files (2017)

#218

Post by deergomoo »

corytheboyd wrote: post #192
Woansdei wrote: post #177

Earlier quoted context omitted.

Sometimes there is nothing you can do when there is an error, in that case there is no point in adding several layers of error forwarding until you ignore it somewhere higher up.

Is… NOT ignoring errors just not an option? I don’t get it. If you propagate errors up but not all the way to being handled, haven’t you failed in a very simple, easy to fix way? Should you have a linter catching these things?

In this case the issue is that defer is a very good way to ensure you don’t forget to close the file in any branches, but a bad way to return values (you have to set the value of a named return variable, which is one of Go’s odder features).

> Should you have a linter catching these things?

JetBrains’ GoLand will in fact warn you of this. If the error truly is immaterial you can instead do

defer func() { _ = f.Close() }()

which is verbose but explicit in its intent to ignore the error.

Re: Don't defer Close() on writable files (2017)

#219

Post by nitwit005 »

russdill wrote: post #207

On production systems, it may often be better to completely ignore the problem at this level. On modern hardware if a disk is throwing an io error on write you are having a bad day. I can almost guarantee that while you might happily modify your code so it properly returns the error, there almost certainly aren't test cases for ensuring such situations are handled "correctly", especially since the error will almost c…

A write failing is totally normal. Unplug an external hard disk.

Re: Don't defer Close() on writable files (2017)

#220

Post by cryptonector »

TickleSteve wrote: post #187
jiggawatts wrote: post #21

Earlier quoted context omitted.

Crazy is the right term. File system APIs in general have too many sharp edges and need a ground-up rethink. Consider S3-like protocols: these recognise that 99% of the time applications just want “create file with given contents” or “read back what they’ve previously written.” The edge cases should be off the beaten path, not in your way tripping you up to when you want the simple scenario.

Is the filesystem the correct abstraction? For most applications, a database-like API is more appropriate, hence SQLite.

The filesystem is very easy to use for simple things by comparison to a DB, and it's more accessible from the shell. But you're right, the filesystem is very difficult to use in a power failure safe way. SQLite3 has great power failure recovery testing, so the advice to use SQLite3 for any but the simplest things is pretty good.

It'd be very nice to get some sort of async filesystem write barrier API. Something like `int fbarrier(int fd)` such that all writes anywhere in the filesystem will be sync'ed later when you `fsync()` that fd.

It would also be very nice to have an async `sync()`/`fsync()`. That may sound oxymoronic, but it's not. An async `sync()`/`fsync()` would schedule and even start the sync and then provide a completion notice so the application can do other work while the sync happens in the background. One can do sync operations in worker threads and then report completion, but it'd be nice to have this be a first class operation. Really, every system call that does "I/O" or is or can be slow should be / have been designed to be async-capable.

Post reply on HN