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

261–270 of 311 posts

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

#261

Post by randomdata »

jrockway wrote: post #245

I defer these AND check the errors. https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functio... has a nice API. I wrote my own version for $WORK package (as our policy is that all errors must be wrapped with a message), used like: func foo() (retErr error) { x := thing.Open(name) defer errors.Close(&retErr, x, "close thing %v", name) ... } You can steal the code from here: https://github.com/pachyderm/pachyderm/b…

With your package, how do you suggest users of your function handle the error upstream?

Like this?

   switch {
   case strings.Contains(err.Error(), "close thing foo"):
       // deal with foo close error
   case strings.Contains(err.Error(), "close thing bar"):
       // deal with bar close error
   }
And if so, what lead you or your organization to prefer "stringly-typed" errors over the idiomatic approach?

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

#262

Post by defrost »

russdill wrote: post #209
defrost wrote: post #162

Earlier quoted context omitted.

Fails if (say) OS can't write out pending cache and confirm data written to device. Causes include memory failure, drive cable melted, network cable pulled, etc. What to do? How important is the data being written? Is the only copy of just aquired data from a $10 million day geophysical survey? How much time and resources can you spend on work arounds, multiple copies, alternative storage paths, etc. In aquisition yo…

Handling it this way in a user process is insane and essentially cargo culting. If your data is that valuable, you have redundant systems.

Custom hardware, custom real time kernel (acquisition|processing DSP boards) + loadable RT firmware, custom kernel + comms + window manager on main terminal.

Other than the recording redundancies described (raw analog logged, raw digital logged, raw paper chart created, cooked data logged, cooked paper chart, (raw | cooked each on tape, disk, paper) what are these "redundant systems" that you speak of?

Keeping in mind, of course, that the client has raw data, etc. on the contract as deliverables.

Do you imagine two full ships pulling two full microphone arrays to offset a rare (but happens) recoring failure? Now you've doubled the per dium costs and halved the area that can be covered in a typically short season.

Do you imagine one ship pulling two arrays that magically don't tangle? It doesn't work that way.

The goal here, of course, is to do all that as feasibly possible upfront in order to minimise aquisition time on the water and to ensure that all pings | booms | etc and their returns to multiple mic's recorded so the ship doesn't have to do a repass.

Expand on your non cargo culting non insane design ideas for 1970-1990s offshore seismic exploration by all means as what you intend isn't clear in your terse comment.

Keep in mind your design will need to be moved on and off arbitrary ships and will operate in places like the North Sea, Spratly Islands, etc. and will have to survive the pitch and toss of stormy weather (eg: attention to card fit in bus backbone).

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

#263

Post by nightowl_games »

randomdata wrote: post #206
Cthulhu_ wrote: post #186

Earlier quoted context omitted.

Exactly that; for critical operations like e.g. a database, if a write fails you've got corrupted data and you have a Major Issue. That said, I'm not sure how they would handle a file close failure, wouldn't the file be corrupted anyway because some of the bits may have been written? Then again, at least you can raise the alarms if Close fails, because silent failures are worse than failures.

> if a write fails. We're talking about a read-only case. os.Open returns a read-only file handle. If you try writing to it, you'll get an error already at that point. If close fails, who cares? > I'm not sure how they would handle a file close failure Ideally there is some kind of failover you can resort to, but if there is no other option at very least you will want to notify a human that what they thought was writ…

I don't know how you can confidently say "if close fails, who cares" all over this thread.

If close fails, I wanna know and I wanna know why.

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

#264

Post by TheDong »

randomdata wrote: post #261
jrockway wrote: post #245

I defer these AND check the errors. https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functio... has a nice API. I wrote my own version for $WORK package (as our policy is that all errors must be wrapped with a message), used like: func foo() (retErr error) { x := thing.Open(name) defer errors.Close(&retErr, x, "close thing %v", name) ... } You can steal the code from here: https://github.com/pachyderm/pachyderm/b…

With your package, how do you suggest users of your function handle the error upstream? Like this? switch { case strings.Contains(err.Error(), "close thing foo"): // deal with foo close error case strings.Contains(err.Error(), "close thing bar"): // deal with bar close error } And if so, what lead you or your organization to prefer "stringly-typed" errors over the idiomatic approach?

Stringly typed errors are also idiomatic in go, if you follow the stdlib.

Like, are you doing anything with TLS? String matching: https://github.com/golang/go/issues/35234

Using the stdlib ssh stuff? String matching: https://github.com/golang/go/issues/45207 / https://github.com/golang/go/issues/39259

Want to parse an address + port? netip.ParseAddrPort only returns strings ('errors.New' errors).

http/http2 is also a minefield of half-exported errors.

The go authors say to use 'errors.Is' and 'errors.As', but the go stdlib also defines an idiom, and the idiom it defines is that somewhere around 30% of all errors should be stringly typed, including many where you may want to have specific handling for them.

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

#265

Post by randomdata »

admax88qqq wrote: post #200
Cthulhu_ wrote: post #180

Earlier quoted context omitted.

Exceptions aren't exceptional though; they are too expensive for not-exceptional errors, like failing writes. That said, a language feature where you can throw lightweight error values without generating a stack trace etc might be a middle ground. But it won't land in Go, given the discussion about alternative error handling some years ago. Anyway, in practice it's not that bad. A write can go wrong, you as a develop…

Exceptions are slow in some languages based upon how they are implemented. I'm not convinced that is fundamental to exceptions and rather a choice of how they were implemented. In Java exceptions arn't actually that slow, most of the cost is just allocating the exception object (and allocations in Java are fast). > Exceptions lead a developer to miss errors, or to not handle them in a finegrained manner - e.g. generi…

> which to be honest is the _correct_ logic in many cases

It is almost never the correct logic. The only time it might be appropriate is in a private helper function that has limited scope around another function.

It is most definitely not the correct logic if you are returning that from a public function! For many reasons, but especially because it now binds you to the implementation of the function you called forevermore. That is a horrible place to be.

For example, find out your os.File usage would be better served by SQLite? Too bad. You can't change it now because the users of your function have come to rely on errors from the os file operations when they handle the error you give them. Their code can't deal with the errors coming out of SQLite.

Instead, you need to return errors that are relevant to your function. It may be appropriate to wrap the source error in some circumstances, but your error structures should compel the user to rely on your errors, leaving the wrapped error only for things like logging where a change in the future won't break the callers.

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

#266

Post by randomdata »

TheDong wrote: post #264
randomdata wrote: post #261

Earlier quoted context omitted.

With your package, how do you suggest users of your function handle the error upstream? Like this? switch { case strings.Contains(err.Error(), "close thing foo"): // deal with foo close error case strings.Contains(err.Error(), "close thing bar"): // deal with bar close error } And if so, what lead you or your organization to prefer "stringly-typed" errors over the idiomatic approach?

Stringly typed errors are also idiomatic in go, if you follow the stdlib. Like, are you doing anything with TLS? String matching: https://github.com/golang/go/issues/35234 Using the stdlib ssh stuff? String matching: https://github.com/golang/go/issues/45207 / https://github.com/golang/go/issues/39259 Want to parse an address + port? netip.ParseAddrPort only returns strings ('errors.New' errors). http/http2 is also a…

> Stringly typed errors are also idiomatic in go, if you follow the stdlib.

Realistically, you can't follow the standard library, except perhaps the newest additions. Idioms emerge and evolve with use. Much of the standard library was written before Go saw much use, being largely in place before the world got to see Go for the first time. Also, thanks to the Go1 guarantee, cannot be changed now.

If the aforementioned package was written in the 2000s, then it might be fair to say that it was in line with the idioms of the time. But it appears to have been written within the last year, and thus is not aligned with idioms of its age.

That's not to say it has to be. Idioms are not requirements. The "stringly-typed" design may be justified even knowing what we know in the 2020s. And, with that, we don't need to speculate about the justifications. We can let the author speak for himself as to why the choice was made.

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

#267

Post by randomdata »

nightowl_games wrote: post #263
randomdata wrote: post #206

Earlier quoted context omitted.

> if a write fails. We're talking about a read-only case. os.Open returns a read-only file handle. If you try writing to it, you'll get an error already at that point. If close fails, who cares? > I'm not sure how they would handle a file close failure Ideally there is some kind of failover you can resort to, but if there is no other option at very least you will want to notify a human that what they thought was writ…

I don't know how you can confidently say "if close fails, who cares" all over this thread. If close fails, I wanna know and I wanna know why.

Let me introduce you to the question mark, seen on your screen as the '?' character. It indicates that a question is asked. This is important as...

Never has "who cares" been confidently said. It has always been asked "who cares?". And not asked in a vacuum either, but specifically asked alongside the question of what is to be gained from the knowledge of the error.

We now know that you allegedly care, which is a promising start. But you purposefully ignored the other question, which questions the credibly of your care. You can't meaningfully care about something if you don't know why you care about it, and if you knew you'd have told us about it already as it nonsensical to answer to the "who cares?" question alone, so...

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

#268

Post by jrockway »

randomdata wrote: post #261
jrockway wrote: post #245

I defer these AND check the errors. https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functio... has a nice API. I wrote my own version for $WORK package (as our policy is that all errors must be wrapped with a message), used like: func foo() (retErr error) { x := thing.Open(name) defer errors.Close(&retErr, x, "close thing %v", name) ... } You can steal the code from here: https://github.com/pachyderm/pachyderm/b…

With your package, how do you suggest users of your function handle the error upstream? Like this? switch { case strings.Contains(err.Error(), "close thing foo"): // deal with foo close error case strings.Contains(err.Error(), "close thing bar"): // deal with bar close error } And if so, what lead you or your organization to prefer "stringly-typed" errors over the idiomatic approach?

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.

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

#269

Post by jrockway »

randomdata wrote: post #258
jrockway wrote: post #255

Earlier quoted context omitted.

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…

There isn't a bug, it's resource exhaustion. You open a bunch of files and they fail to close. You don't log errors on the close, so you have no idea it's happening. Now your app is failing to open new file descriptors to accept HTTP connections. You get a fixed number of fds per app; ulimit -n. If you don't close files you've read, the descriptor is gone.

The bug in this case is in the filesystem that hangs on close. It happens on network filesystems. You can't return the fd to the kernel if your filesystem doesn't let you.

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

#270

Post by randomdata »

jrockway wrote: post #268
randomdata wrote: post #261

Earlier quoted context omitted.

With your package, how do you suggest users of your function handle the error upstream? Like this? switch { case strings.Contains(err.Error(), "close thing foo"): // deal with foo close error case strings.Contains(err.Error(), "close thing bar"): // deal with bar close error } And if so, what lead you or your organization to prefer "stringly-typed" errors over the idiomatic approach?

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 string matching to protect against undocumented functionality and/or future modifications when you handle the error. Sounds like a rough situation...

> If I'm going to do something upstream based on the error, I use a type.

Implying that you are a lone wolf developer? I think you make a good point that if you exist in your own world without other developers just about anything goes.

That said, the language used around the previously linked repository implies that it welcomes other developers using and working on the code, so it is not clear how you are "doing the upstream" in all cases.

Post reply on HN