Live data from Hacker News

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

joeshaw.org

91–100 of 311 posts

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

#91
post #8

Earlier quoted context omitted.

Surely File.close will clear the fd on success...

Yeah but is it safe for concurrent use?

If you’re performing concurrent unprotected closing of an fd, you’re deep into the wild lands of nonsense already.

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

#92
post #90
post #37

AFAIK Python/C# use a similar approach to Go - instead of `defer`, they have `using`/`with` statements. Go's `defer` seems more flexible though - it can execute custom code each time it's used, whereas `using`/`with` always call `__exit__`/`Dispose`. How does the Python/C# approach compare to Go's in this situation? How are errors in `__exit__`/`Dispose` handled?

> Exceptions that occur during execution of this method will replace any exception that occurred in the body of the with statement https://docs.python.org/3/library/stdtypes.html#contextmanag...

Thanks!

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

#93
post #23

Here is my favorite solution to this problem // CheckClose is a utility function used to check the return from // Close in a defer statement. func CheckClose(c io.Closer, err *error) { cerr := c.Close() if *err == nil { *err = cerr } } Use like this - you must name the error return func whatever() (err error) { f, err := os.Open(blah) // ... defer CheckClose(f, &err) // ... } This closes the file and if there wasn't…

I still question why defer doesn't support doing exactly that. After all it's like the go language provide us with a cleanup function that in 99% of the time shouldn't be used unless we manually wrap what it's calling to properly handle error. In the end, what's the point of defer ?

> I still question why defer doesn't support doing exactly that.

When would it ever be useful? You'd soon start to hate life if you actually tried using the above function in anything beyond a toy application.

> 99% of the time shouldn't be used

1. 99% of the time it is fine to use without further consideration. Even if there are errors, they don't matter. The example from the parent comment is a perfect case in point. Who cares if Close fails? It doesn't affect you in any way.

2. 0.999% of the time if you have a function that combines an operation that might fail in a manner you need to deal with along with cleanup it will be designed to allow being called more than once, allowing you, the caller, to separate the operation and cleanup phases in your code.

3. 0.001% you might have to be careful about its use if a package has an ill-conceived API. If you can, fix the API. The chances of you encountering this is slim, though, especially if you don't randomly import packages written by a high school student writing code for the first time ever.

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

#94
post #87

Earlier quoted context omitted.

I think some of those issues with errors could be worked around. Wrapping errors provides a way to return a "cleanup failed" error that also includes the root cause of being a failed write. Likewise, there are packages for handling multi-errors. I think the reality is that most of us push anything where the return status of Close would be important into the database, specifically because it handles semantics like thi…

I mean, we can cause the same kind of problem there, as people might try to write a scope exit handler to commit a transaction. You'd then run into the same issue, and so "commit transaction" isn't a thing which should ever be in such a construct. Of course, deleting the objects for the transaction / closing the database connection / etc. would be fine to ignore errors from (and hopefully wouldn't/shouldn't fail anyw…

I don't think that holds for database transactions because of the semantics of rollbacks. Trying to do a rollback after committing is effectively a no-op (I believe it returns an error, but doesn't actually change the DB), so you can defer a rollback and only call commit on the happy path.

I don't believe people generally care about the error context on the rollback, which makes it safe to defer into a context that can't interact with the error handling monads. Rollbacks shouldn't generally fail, even if they do there's basically nothing you can do about it, and there are few differences between a successful and failed rollback beyond resources on the DB server until the connection is closed.

The Commit is the portion that contains the context people care about in their errors, and that is still safely in a context where it can interact with error handling.

I believe files can get similar atomicity, but it requires doing IO in strange ways. E.g. updating a file isn't atomic, but mv'ing one is. So you can copy the file you want to update into /tmp, update the copy, and then mv the copy to the original file (commit is mv'ing it, rollback is rm'ing it or just ignoring it).

Database transactions aren't atomic and do have the same issue if they reference external resources, though. E.g. if you have a database that stores an index of S3 files, transactions won't save you from writing a file to S3 but then failing to write a record for it into the database. That does muddle the error handling again.

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

#95

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?

That should be only for creating files, and maybe updating their metadata (not sure about that one). The confusion stems from people thinking that files and directories are more different than they are. Both are inodes, and both are basically containers for data. File inodes are containers for actual data, while directory inodes are containers for other inodes. All inodes need to be fsynced when you write to them. Fo…

> It’s basically a graph.

do you mean it's a basically a tree? Because if it were just a graph, you could still have edges from the grandparent to the grandchild in addition to the one from the former to the child

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

#96

Earlier quoted context omitted.

That should be only for creating files, and maybe updating their metadata (not sure about that one). The confusion stems from people thinking that files and directories are more different than they are. Both are inodes, and both are basically containers for data. File inodes are containers for actual data, while directory inodes are containers for other inodes. All inodes need to be fsynced when you write to them. Fo…

> It’s basically a graph. do you mean it's a basically a tree? Because if it were just a graph, you could still have edges from the grandparent to the grandchild in addition to the one from the former to the child

This ... depends. In normal POSIX land, hard links break the tree structure, for one, so you get a DAG but not a tree. I think some file systems do enforce tree structure, though - hard links are not supported everywhere.

It used to be possible ages ago to hard link to directories, which meant that you could have actual cycles and a recursive tree-walking algorithm would never terminated. (As far as I know you can still do this by editing the disk manually, although I think fsck will make a fuss if it detects this.)

You can still, with the right syscalls and drivers, do something like hard links on NTFS (I think they're technically called mount points but it's not the same thing as POSIX ones). I'm not sure if you can still make directory cycles, and you're probably a bad person if you do.

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

#97
post #26

Surely this would all go away if Go had an exception handling mechanism like most mainstream languages do? You'd just concentrate on the "happy path", you'd close the file, there'd be nothing to forget or write blog posts about because the exception would be propagated, without needing to write any lines of code.

This problem isn't solved with exceptions either. The problem is that finalizers (C++ destructors, Java's `finally` blocks, Go's `defer` etc.) shouldn't fail but `close()` can fail. Therefore, for 100% correctness, `close()` calls should be handled explicitly and not left to finalizers. Finalizers shouldn't fail because they might be executed while another exception is already in flight. Three languages have three di…

Python handles this case by raising the new error but including a reference to the original error. By default, the formatted error shows both:

    >>> mylist = []
    >>> try:
    ...     first = mylist[0]
    ... finally:
    ...     inverse_length = 1.0 / len(mylist) # imagine this was something more complex
    ... 
    Traceback (most recent call last):
      File "", line 2, in 
    IndexError: list index out of range
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "", line 4, in 
    ZeroDivisionError: float division by zero

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

#98

Earlier quoted context omitted.

> So if I do fopen/fwrite/fsync/fclose, that is not enough? That is my understanding. > Also, how many levels of parents do you need to fsync? Only one, at least if you didn't create the parent directory (if you did then you might have to fsync its parent, recursively). The fsync on the parent directory ensures the dir entry for your new file is flushed to disk.

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 edge-case-handling code for different OSes.

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

#99

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?

One of the advantages of using a database for files is that it's relatively more likely that these platform-dependent considerations were indeed considered and you don't have to moonlight as an DBMS engineer while writing your application.

Hence the saying, "SQLite competes with fopen()".

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

#100
post #23

Here is my favorite solution to this problem // CheckClose is a utility function used to check the return from // Close in a defer statement. func CheckClose(c io.Closer, err *error) { cerr := c.Close() if *err == nil { *err = cerr } } Use like this - you must name the error return func whatever() (err error) { f, err := os.Open(blah) // ... defer CheckClose(f, &err) // ... } This closes the file and if there wasn't…

I wouldn't mind the language adding some syntax for this, based on the java try-with-resource but using error values rather than exceptions. Something like

    f, err = with(os.Open(thing)) {
        // do stuff
    }
    // at this point err has the same semantics as your CheckClose
Post reply on HN