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

251–260 of 311 posts

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

#251

Post by aseipp »

Cthulhu_ wrote: post #179
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.

Aren't the edge cases features? An abstraction (or different API, sure) is in order to prevent footguns. However, this abstraction should not force fsyncs for example, due to the performance impact mentioned. It puts the choice of guaranteed writes vs performance to the developer.

> Aren't the edge cases features?

What features do you have in mind?

> It puts the choice of guaranteed writes vs performance to the developer.

Yes, and it's a completely false choice. This entire point of this thread is that fsync is an incredibly difficult API to use in a way that gets you the guarantees you need ("don't lose the writes to this file"). And that the consistency guarantees of specific filesystems, VFS, POSIX, and their interactions are not easy to understand even for the experienced -- and it can be catastrophic to get wrong.

It isn't actually a choice between "Speed vs correctness". That's a nice fairy tale where people get to pretend they know what they're up against and everyone has full information. Most programmers aren't going to have the attention to get this right, even good ones. So then it's just "99.9% chance you fucked it up and it's wrong" and then your users are recovering data from backups.

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

#252

Post by josephg »

Cthulhu_ wrote: post #179
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.

Aren't the edge cases features? An abstraction (or different API, sure) is in order to prevent footguns. However, this abstraction should not force fsyncs for example, due to the performance impact mentioned. It puts the choice of guaranteed writes vs performance to the developer.

A better abstraction, designed from the ground up, wouldn’t force fsyncs to work.

For example, write groups or barriers (like memory barriers) would be wonderful. Or a transaction api, or io completion ports like on windows.

In a database (and any other software designed for resiliency), you want the file contents to transition cleanly from state A to B to C, with no chance to end up in some intermediate state in the case of power loss. And you want to be notified when the data has durably written. It’s unnecessarily difficult to write code that does that on top of POSIX in an efficient way. Most code that interacts with files is either slow, wrong or both. All because the api is bad.

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

#253

Post by randomdata »

jrockway wrote: post #246
randomdata wrote: post #145

Earlier quoted context omitted.

You seem confused. The article is about writing a file where it does matter, but the comment example, which is what we're talking about, only reads a file. If close fails after read, who gives a shit? What difference is it going to make? All your read operations are complete already. Close isn't going to trigger a time machine that goes back and time and undos the reads you've performed. It is entirely inconsequentia…

> If close fails after read, who gives a shit? ulimit -n You ignore errors on close, and one morning you wake up with your app in CrashLoopBackoff with the final log message "too many files". How do you start debugging this? Compare the process to the case where you do log errors, and your log is full of "close /mnt/some-terrible-fuse-filesystem/scratch.txt: input/output error". Still baffling of course, but you have…

To start, you need to figure out why Kubernetes isn't retaining your stack trace/related metadata when the app crashes. That is the most pressing bug. Which is probably best left to the k9s team. You outsourced that aspect of the business of good reason, no doubt.

After they've fixed what they need to fix you need to use the information now being retained to narrow down why your app is crashing at all. Failing to open a file is expected behaviour. It should not be crashing.

Then maybe you can get around to looking at the close issue. But it's the least of your concerns. You've got way bigger problems to tackle first.

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

#254

Post by randomdata »

foldr wrote: post #244
randomdata wrote: post #242

Earlier quoted context omitted.

> the lesson to be drawn from that is "don't defer a function call if you need to check its error value" Isn't the lesson here: If you must have a Close method that might fail in your API, ensure it can safely be called multiple times? As long as that is true, you can approach it like you would any other API that has resources that might need to be cleaned up. f, _ := os.Create(...) defer f.Close() // perform writes…

Sure, that's an alternative, although it means there will be some code paths where the error returned by f.Close() becomes the error returned by the entire function and others where it is ignored (though you could easily log it). That might be fine, but you also might want to handle all the cases explicitly and return a combined error in a case where, say, a non-file-related operation fails and then the file also fai…

> becomes the error returned by the entire function

If you find the error returned by f.Close to be significant, are you sure returning again it is the right course of action? Most likely you want to do something more meaningful with that state, like retrying the write with an alternate storage device.

Returning the error is giving up, and giving up just because a file didn't close does not make for a very robust system. Not all programs need to be robust, necessarily, but Go is definitely geared towards building systems that are intended to be robust.

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

#255

Post by jrockway »

randomdata wrote: post #253
jrockway wrote: post #246

Earlier quoted context omitted.

> If close fails after read, who gives a shit? ulimit -n You ignore errors on close, and one morning you wake up with your app in CrashLoopBackoff with the final log message "too many files". How do you start debugging this? Compare the process to the case where you do log errors, and your log is full of "close /mnt/some-terrible-fuse-filesystem/scratch.txt: input/output error". Still baffling of course, but you have…

To start, you need to figure out why Kubernetes isn't retaining your stack trace/related metadata when the app crashes. That is the most pressing bug. Which is probably best left to the k9s team. You outsourced that aspect of the business of good reason, no doubt. After they've fixed what they need to fix you need to use the information now being retained to narrow down why your app is crashing at all. Failing to ope…

The app crashes because "too many files" includes the fd accept(2) wants to allocate so your app can respond to the health check.

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

#256

Post by slaymaker1907 »

Best practice IMO would be to wrap the closable thing in a wrapper object that handles the case where Close is called multiple times and then defer close that one as well as closing at the end of the function. Another idea would be to define a small lambda that either returns the input value or returns error if Close returns an error if you want to use early returns.

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

#257

Post by Veserv »

yyyfb wrote: post #172

Boggles my mind that after more than 60 years of computer science, we still design tools (programming languages) where the simplest tasks are full of gotchas and footguns. This is a great example.

The problem is that the filesystem primitives are garbage, so it is impossible to make something safe and reasonably performant. This is not a case of "speed at all costs" where huge footguns are added for marginal performance, this is avoiding 10x and up slowdowns that would be required to be safe due to the anemic primitives. If the filesystem had better primitives/APIs, like barriers and proper asynchronous completion, it would be trivial to design tools that are safe and performant. But, without them it is like trying to build a skyscraper out of mud and toothpicks.

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

#258

Post by randomdata »

jrockway wrote: post #255
randomdata wrote: post #253

Earlier quoted context omitted.

To start, you need to figure out why Kubernetes isn't retaining your stack trace/related metadata when the app crashes. That is the most pressing bug. Which is probably best left to the k9s team. You outsourced that aspect of the business of good reason, no doubt. After they've fixed what they need to fix you need to use the information now being retained to narrow down why your app is crashing at all. Failing to ope…

The app crashes because "too many files" includes the fd accept(2) wants to allocate so your app can respond to the health check.

A file not able to opened is expected, always! accept is no exception here. Your application should not be crashing because of it.

If I recall, Kubernetes performs health checks over HTTP, so presumably your application is using the standard library's http server to provide that? If so, accept is full abstracted away. So, if that's crashing, that's a bug in Go.

Is that for you to debug, or is it best passed on to the Go team?

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

#259

Post by surajrmal »

bqmjjx0kac wrote: post #223
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(); }

A method which takes foo and turns it into another type is slightly better. You can then unconditionally panic in the drop impl.

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

#260

Post by [deleted] »

jrockway wrote: post #247
corytheboyd wrote: post #171

I’m a bit new to Golang, but not good programming practices. Isn’t ignoring the error value returned by a function a very bad practice in general? Regardless of if the function call returning it is used in defer? Not just for file write operations?

Ignoring errors is generally a poor practice. You don't have to stop your program on errors, but you should at least log some percentage of them so that when the failure cascades, you have some idea where the failure started.

[deleted]
Post reply on HN