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

221–230 of 311 posts

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

#221

Post by cryptonector »

red_admiral wrote: post #98
magicalhippo wrote: post #60

Earlier quoted context omitted.

I've never heard about this in my years of programming. I just tried to read through the Win32 documentation, as I've done several times over the years, and it mentions a lot of edge cases but not this that I could see. Is this some Linux/Unix specific thing? Am I blind?

As far as I know, it's specific to the combination of certain POSIX-ish OS and file systems, like linux/Ext3. I have no clue what BSD does here, or whether ReiserFS is different. Windows/NTFS is a different world, there are still edge cases that can go wrong but I don't think this particular one is a problem because FAT/NTFS is not inode-based. I imagine if you looked at the SQLite source code you'd see different edg…

NTFS has inodes.

The thing about Windows is that because the file open operation (`CreateFile*()`) by default prevents renames of the file, Windows apps have come to not depend so much on file renaming, which makes one of the biggest filesystem power failure hazards less of an issue on Windows. But not being able to rename files over others as easily as in POSIX really sucks. And this doesn't completely absolve Windows app devs of having to think about power failure recovery! "POSIX semantics" is often short-hand for shortcomings of POSIX that happen to also be there in the WIN32 APIs, such as the lack of a filesystem write barrier, file stat-like system calls that mix different kinds of metadata (which sucks for distributed filesystems protocols), and so on. And yes, you can open files in Windows such that rename-over is allowed, so you still have this problem.

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

#222

Post by masklinn »

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…

> 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.

Is it though? It ensures the fd is closed which is what you want, and if you have some form of unwinding in the language you can't really ask for more. And aborts are, if anything, worse.

It also works perfectly well for reading, there's no value to close errors then.

> Closing a file can raise errors but you can't reasonably treat errors on drop.

It's mostly useless anyway, since close does not guarantee that the data has been durably saved. If you want to know that, you need to sync the file, and in that case errors on close are mostly a waste of time:

- if you've opened the file for reading you don't care (errors are not actionable, since you can't retry closing on error)

- if you've flushed a write, you don't care (for the same reason as above)

The one case where it matters is if you care but missed it, in which case we'd need a bunch of different things:

- a version of must_use for implicit drops

- a consuming flush-and-close method on writeable files

- a separate type for readable and writeable files in order to hook both, and a suite of functions to convert back and forth because even if rust had the subtyping for you don't want to move from write to read without either flushing or explicitly opting out of it as you're moving into a "implicit drop is normal" regime

> 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 variable

That is nonsensical, the entire point of drop is that it's a hook into default / implicit behaviour. How do you "handle errors" when a drop is called during a panic? It also doesn't make sense from the simple consideration that you can get drops in completely drop-unaware code.

Consuming methods is what you're looking for.

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

#223

Post by bqmjjx0kac »

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…

You can kind of achieve this at runtime like so

    struct Foo {
      bool dirty,
    }

    impl Foo {
      fn clean_up(&mut self) {
        // ...
        self.dirty = false;
      }
    }

    impl Drop for Foo {
      fn drop(&mut self) {
        if self.dirty {
          panic!("Foo was not cleaned up before its lifetime ended");
        }
      }
    }

    fn bar(foo: &mut foo) {
      foo.clean_up();
    }

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

#224

Post by cryptonector »

garrettr_ wrote: post #183
Verdex wrote: post #178

Earlier quoted context omitted.

Great another case of simple thing everyone knows is simple but turns out to be horrifyingly complicated and heavily system dependent, but only on occasion so you can get 80% of the way through your career before encountering the gaps in your knowledge. I guess I'll add it to the list. Of course on the other hand, I was already thinking that I should just use SQLite for all my file handling needs. This little nugget…

w.r.t SQLite, the only horrifying revelation I’ve had is that it allows NULLs in composite primary keys, which I’ve seen lead to some nasty bugs in practice.

Yes, you have to declare PRIMARY KEY columns as NOT NULL. There's lots of little caveats like this about SQLite3. So what.

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

#225

Post by jryan49 »

weinzierl wrote: post #202
jryan49 wrote: post #157

Earlier quoted context omitted.

This says the bug is fixed? https://bugs.openjdk.org/browse/JDK-8066915

The point is that there is no official way to fsync a directory in Java and that everyone is relying on an unintentional side effect of an unrelated function to accomplish it. The link I supplied is about the fact that the side effect briefly disappeared in Java 9 until enough people complained. We're still living in xkcd 1172 land with this, have been for a decade or who knows how long.

Java does "abstract" the operating system away from you and in systems programming with java you can end up with leaky abstractions.

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

#226

Post by Brian_K_White »

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…

"listen to what close() says"

and

"don't believe what close() says"

are two different things.

The article is only (initially) talking about the first, and that is valid.

The second is just second-guessing the OS and hardware environment, and is invalid.

Here's the rule to figure out if you need fsync() or not: "If you think you might need fsync(), you don't." ;)

There are almost no cases where you should worry about the underlying layers. Basically if you aren't writing the filesystem itself, then you shouldn't be calling fsync().

You have to check what open()/close() etc said, but if close() said it worked, then it worked. You're done. The fact that lightning might have struck the drive just exactly then is not your problem and not something you should try to do anything about.

Unreliable networks, busses, batteries, etc none of that changes this. Those things all already have their own layers with their own responsibilities to be doing all the necessary testing and verifying and retrying before they return a success code to you.

There is open(...,O_SYNC) and mount -o sync (udev rules) for the case of a camera or thumb drive connect by usb etc.

It's not merely that you don't have to, it's that it's actively wrong to. fsync() is just joggling someone else's elbow while they're trying to do their job, and if it seems to solve some problem, that actually just exposes that you have some logic or order of operations problem and you aren't doing your own job.

"trust the other layer" or "trust the api contract" is unrelated to and does not conflict with "Be forgiving in your inputs and strict in your outputs.".

It just means:

Do: Check the returned error from, say, malloc().

Don't: Get a success from malloc() and then go try to do things to prove that malloc() actually did work.

That would be insane and impossible because it would have to apply equally to everything, including every single keyword or function you would use as part of the verification. How do you know when you so much as set a value to variable that it actually got set? If you printf the variable to prove it, how do you know printf didn't lie?

The logic for trusting close() is no different from the logic for trusting malloc().

Our responsibility is just to stay within the bounds of defined behavior and not make any assumptions about anything that isn't promised.

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

#227

Post by bornfreddy »

vlovich123 wrote: post #198
nextaccountic wrote: post #182

Earlier quoted context omitted.

> It also goes the other way - the update to the directory can be fsynced but the file lost. This can break the "create temp file, write, close, rename to current" scenario (when the intention is to replace file contents atomically). > POSIX doesn't guarantee the order in which data hits the disk, so the above scenario can become "create temp file, write [contents still in memory only], rename to current [written to…

Write, fsync, rename then fsync directory if you need an ordering guarantee that the rename is a transaction barrier. Of course, the fun part is that the filesystem can’t really guarantee fsync behavior if drives lie about it which many consumer drives do for benchmark reasons. Fun, no?

It would also be a fun experiment to write, fsync, cut power. Rinse and repeat. On differential drives. It would quickly show which drives are lying about syncing.

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

#228

Post by nine_k »

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.

But the authors of SQLite have studied and correctly implemented the working fsync logic, so you don't have to. It ·may* be a correct approach if it's your implementation detail, and.nobody else expects that file you're replacing.

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

#229

Post by kaba0 »

lostmsu wrote: post #81
neonsunset wrote: post #78

Earlier quoted context omitted.

I'm catching any exception that occurs within the scope of try block. This includes an exception when trying to open or write to the file. If I fail to write to a file, it will try to dispose the stream, which flushes it and closes the file handle. If disposing the file handle itself fails, which should never happen, the exception will occur in the finally block, which this exception handler catches too. If you need…

Yes, and this ignoring of the original exception is the core of the problem discussed. If you are willing to lose supposedly written data, your approach is golden.

With exceptions you don’t silently ignore error and go on as if nothing happened.

Just simply not explicitly handling every single possible error is the correct choice in many scenarios - in which case it bubbles up to a general error handler, e.g. telling the user that something bad happened here and here.

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

#230

Post by spease »

assbuttbuttass wrote: post #141
spease wrote: post #138

Isn’t what the article suggests (with defer and then WriteString) technically a race condition? Is there no way that the closer can get called before WriteString executes?

defer gets executed at the end of the current function

Thanks! Not a Go programmer, so I saw the parentheses after the cloaure def and assume it translated to “execute this on a goroutine in the background immediately”
Post reply on HN