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

291–300 of 311 posts

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

#291

Post by jrockway »

randomdata wrote: post #288
jrockway wrote: post #274

Earlier quoted context omitted.

You're really hung up on Kubernetes but it was an incidental comment in a hypothetical story. "You wake up and find out that Heroku's staff is anxiously awaiting your departure from your apartment to tell you that your app is down."

Kubernetes is really here nor there. It's the crashing of the app that is our focus. An app should not be crashing on expected behaviour. That's clearly a bug, and the bug you need to fix first so that you can have your failsafes start working again. You asked where to start and that's the answer, unquestionably.

The app doesn't crash, it's deadlocked. It can't do any more work because to do future work it needs to accept TCP connections. It can't do that because it has hit a resource limit. It hit the resource limit because it didn't correctly close files. It can't close files because of a bug in the filesystem. You don't know this because you didn't log the errors.

I really don't know how I can make my explanation simpler.

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

#292

Post by jrockway »

randomdata wrote: post #270
jrockway wrote: post #268

Earlier quoted context omitted.

errors.Is is true for whatever x.Close() returned. If I'm going to do something upstream based on the error, I use a type. Most of the time, it's just log.Error text though.

> errors.Is is true for whatever x.Close() returned. That may get you through if there is only one x.Close, but now you have to offer the guarantee to the callers that there will only ever be one Close error returned. Furthermore, any callers of your function have to ensure that they don't end up introducing additional Close error returns in the same vein. Without such guarantees, you, the caller, need to resort to s…

> That may get you through if there is only one x.Close, but now you have to offer the guarantee to the callers that there will only ever be one Close error returned.

No you don't. It's a multierror, a feature of the standard library.

    errors.Is(
        errors.Join(ErrA, ErrB, ErrC, errors.Join(ErrD, ErrE, ErrF)),
        ErrF)
is true. This is a feature in the standard library.

> Implying that you are a lone wolf developer?

You know that's not true from the history of the repository. We do have coding standards that reviewers enforce, this is one of them.

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

#293

Post by lxgr »

o11c wrote: post #213
kccqzy wrote: post #152

Earlier quoted context omitted.

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 cha…

That seems significantly harder to implement, though, given that dirty page cache entries don't have an order entry as far as I know, and so retroactively figuring out which writes not to reorder with others is anything but trivial.

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

#294

Post by lxgr »

Brian_K_White wrote: post #226
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 fact that lightning might have struck the drive just exactly then is not your problem [...] If you think you might need fsync(), you don't. [...] Those things all already have their own layers with their own responsibilities to be doing all the necessary testing [...]

You're mixing up the concepts of durability and consistency in pretty significant ways here, and are implying that everybody that is fine with a lack of the former will also be fine with a lack of the latter.

This is absolutely not true, and can cause extremely painful and hard-to-track-down bugs.

For better or worse, there is no way to directly tell the OS "whatever you do, make sure you don't reorder these writes I just did with those other writes I'm about to do".

The next best (portable) thing we have to achieve that outcome is fsync. It's a bit heavy handed, in that it gives you durability even if you only want consistency. That absolutely doesn't mean it's redundant, though.

> Basically if you aren't writing the filesystem itself, then you shouldn't be calling fsync().

Given that fsync is a syscall, but file systems are generally implemented in the kernel, this is a pretty nonsensical statement by itself.

File systems usually have (and need, for performance) a much lower level view of the underlying block storage, and fine-grained control over it.

Just as one example, Linux has the concept of write barriers. Not using these correctly (in the filesystem driver) can cause data leaks across files owned by different users and processes.

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

#295

Post by randomdata »

jrockway wrote: post #292
randomdata wrote: post #270

Earlier quoted context omitted.

> errors.Is is true for whatever x.Close() returned. That may get you through if there is only one x.Close, but now you have to offer the guarantee to the callers that there will only ever be one Close error returned. Furthermore, any callers of your function have to ensure that they don't end up introducing additional Close error returns in the same vein. Without such guarantees, you, the caller, need to resort to s…

> That may get you through if there is only one x.Close, but now you have to offer the guarantee to the callers that there will only ever be one Close error returned. No you don't. It's a multierror, a feature of the standard library. errors.Is( errors.Join(ErrA, ErrB, ErrC, errors.Join(ErrD, ErrE, ErrF)), ErrF) is true. This is a feature in the standard library. > Implying that you are a lone wolf developer? You kno…

> It's a multierror, a feature of the standard library.

Understood (I read the code), but that doesn't help, and really has nothing to do with the topic at hand. Is the problem here that you don't have an understanding of what we're talking about?

> You know that's not true from the history of the repository.

Which is why the claim was identified as being strange and in need of clarification. How can "If I'm going to do something upstream based on the error, I use a type." be true? If there are many developers, you don't get to control the upstream.

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

#296

Post by randomdata »

jrockway wrote: post #291
randomdata wrote: post #288

Earlier quoted context omitted.

Kubernetes is really here nor there. It's the crashing of the app that is our focus. An app should not be crashing on expected behaviour. That's clearly a bug, and the bug you need to fix first so that you can have your failsafes start working again. You asked where to start and that's the answer, unquestionably.

The app doesn't crash, it's deadlocked. It can't do any more work because to do future work it needs to accept TCP connections. It can't do that because it has hit a resource limit. It hit the resource limit because it didn't correctly close files. It can't close files because of a bug in the filesystem. You don't know this because you didn't log the errors. I really don't know how I can make my explanation simpler.

> The app doesn't crash

You literally said that it crashes:

   The app crashes because "too many files" includes the fd accept(2) wants to allocate so your app can respond to the health check.
https://news.ycombinator.com/item?id=41505892

> I really don't know how I can make my explanation simpler.

Not making up some elaborate story that you now are trying to say didn't even happen would be a good start. What you are actually trying to communicate is not complicated at all. It didn't need a story. Not sure what you were thinking when you decided fiction writing was a good idea, but I certainly had fun making fun of you for it! So, at least it was not all for not.

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

#297

Post by Brian_K_White »

lxgr wrote: post #294
Brian_K_White wrote: post #226

Earlier quoted context omitted.

"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 fact that lightning might have struck the drive just exactly then is not your problem [...] If you think you might need fsync(), you don't. [...] Those things all already have their own layers with their own responsibilities to be doing all the necessary testing [...] You're mixing up the concepts of durability and consistency in pretty significant ways here, and are implying that everybody that is fine with a…

"there is no way to directly tell the OS "whatever you do, make sure you don't reorder these writes I just did with those other writes I'm about to do"

Perhaps because there is no reason for such a thing to exist.

Tell me an example.

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

#298

Post by lxgr »

Brian_K_White wrote: post #297
lxgr wrote: post #294

Earlier quoted context omitted.

> The fact that lightning might have struck the drive just exactly then is not your problem [...] If you think you might need fsync(), you don't. [...] Those things all already have their own layers with their own responsibilities to be doing all the necessary testing [...] You're mixing up the concepts of durability and consistency in pretty significant ways here, and are implying that everybody that is fine with a…

"there is no way to directly tell the OS "whatever you do, make sure you don't reorder these writes I just did with those other writes I'm about to do" Perhaps because there is no reason for such a thing to exist. Tell me an example.

Text editors: I want my file content to either be what it was before I pressed "save", or what it was afterwards; I don't want my file to be mangled halfway through. (In other words, in this case I need consistency, but not necessarily durability.) How would you do that without fsync?

Databases: People generally don't like unrecoverable consistency errors just because their computer crashed during a write. Not generally possible with reordered writes.

Sometimes people also need durability on top of consistency, e.g. for everything where you want to make at most one request to some server; you can do that by e.g. writing "I did the thing" to a log file, fsync'ing it, and then making your request.

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

#299

Post by TheDong »

randomdata wrote: post #287
TheDong wrote: post #286

Earlier quoted context omitted.

> Error type reuse where different failure points produce the same type of error does not violate current idioms. I cannot immediately think of any reason for why their assumption is wrong, so unless you have other ideas? I have a program that takes user input and parses it, and then displays an error. My program is for a language other than english so having it display a pop up with the message "invalid ip:port, squ…

I'm not sure the desire to perform a transformation on a value implies that there are multiple types. You speak to a real problem, of course, but perhaps at the wrong layer of abstraction. It seems the deeper seeded issue is that Go strings assume one language, which is not true. I wonder what native internationalization support might look like?

On multiple types, there's another reason there's multiple types.

Imagine a function that looks like:

    addr, err := netip.ParseAddrPort(input)
    if err != nil { return err }
    err = makeConnection(addr)
    if err != nil { return err }
The caller of this function will now want to distinguish between "Did we fail to parse input, or did we fail to do networking".

The way to do that is to have `netip.ParseAddrPort` failures return a different error type than other methods, but the "idiomatic" go code above doesn't wrap the error with an additional type, so the caller can't distinguish between a network error (many of which are also just 'errors.New'), and a netip parse error (all of which are 'errors.New').

Pushing that responsibility onto the callers seems silly, and like a footgun, especially when several other packages do have typed errors that mean the caller can successfully identify the error without having to do verbose explicit wrapping.

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

#300

Post by randomdata »

TheDong wrote: post #299
randomdata wrote: post #287

Earlier quoted context omitted.

I'm not sure the desire to perform a transformation on a value implies that there are multiple types. You speak to a real problem, of course, but perhaps at the wrong layer of abstraction. It seems the deeper seeded issue is that Go strings assume one language, which is not true. I wonder what native internationalization support might look like?

On multiple types, there's another reason there's multiple types. Imagine a function that looks like: addr, err := netip.ParseAddrPort(input) if err != nil { return err } err = makeConnection(addr) if err != nil { return err } The caller of this function will now want to distinguish between "Did we fail to parse input, or did we fail to do networking". The way to do that is to have `netip.ParseAddrPort` failures retu…

> The way to do that is to have `netip.ParseAddrPort` failures return a different error type than other methods

No. Absolutely not. If you leave the callers of this hypothetical function to rely on the errors of the functions it calls, even if we assume those functions were written by an infallible deity, you've created a coupling that now binds you to the implementation forevermore. That's just plain reckless behaviour.

Return your own errors. I know it takes a tiny amount of extra thinking to figure out what types are relevant to your function, but is unquestionably worthwhile and would still be worthwhile even if all the functions you call were designed by an infallible entity.

> but the "idiomatic" go code

You're really stretching the use of idiomatic here. Not ever has that been considered idiomatic Go. The Go community has always been clear that you should never, ever write code like that. It is so painfully horrid for so many reasons that it could never be considered idiomatic.

Post reply on HN