some incorrect code in blog.

206 views
Skip to first unread message

Fannie Zhang

unread,
Nov 24, 2021, 9:13:49 PM11/24/21
to golang-nuts
Hi all,

There is some incorrect code in https://go.dev/blog/defer-panic-and-recover blog.

The original code is
func CopyFile() {
   ...
   if err != nil {
      return
   }
   defer src.Close()
   ...
}

I think the correct code should be
func CopyFile() {
   ...
   defer src.Close()
   if err != nil {
      return
   }
   ...
}

I do not know how to modify the go blog, can anyone help? Thank you.

Best regards,
Fannie Zhang

Kurtis Rader

unread,
Nov 24, 2021, 9:26:57 PM11/24/21
to Fannie Zhang, golang-nuts
Notice the date of that blog article: 2010-08-04. It's more than eleven years old. Blog articles are not updated as the language changes. However, in this case the example in that article is correct. If `os.Open()` returns an error then the `src` return value is invalid.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/726e95ed-9b14-46a5-bb09-7821616f4ff9n%40googlegroups.com.


--
Kurtis Rader
Caretaker of the exceptional canines Junior and Hank

Leam Hall

unread,
Nov 24, 2021, 10:13:12 PM11/24/21
to golan...@googlegroups.com
On 11/24/21 20:26, Kurtis Rader wrote:
> Notice the date of that blog article: 2010-08-04. It's more than eleven years old. Blog articles are not updated as the language changes.

It might be good to resolve that. If the authoritative info source for the language makes a habit of keeping old data up front, either update the material or quit posting stuff that ages to incorrectness. Winning a 2010 award ages well, code from 2010 might not. New community members don't need the hindrance, do they?

Leam

--
Systems Programmer (reuel.net/resume)
Scribe: The Domici War (domiciwar.net)
General Ne'er-do-well (github.com/LeamHall)

Ian Lance Taylor

unread,
Nov 24, 2021, 11:18:20 PM11/24/21
to Fannie Zhang, golang-nuts
On Wed, Nov 24, 2021 at 6:14 PM Fannie Zhang <Fannie...@arm.com> wrote:
>
If you want to update the blog, send a patch for
golang.org/x/website/_content/blog.

(But please don't send this particular change. As Kurtis noted, the
existing code is correct, and your change would not be correct.
Thanks.)

Ian

David Karr

unread,
Nov 25, 2021, 2:27:01 AM11/25/21
to Fannie Zhang, golang-nuts
On Wed, Nov 24, 2021 at 6:14 PM Fannie Zhang <Fannie...@arm.com> wrote:
Well, I would call myself a Go newbie, but I would think that if err != nil, that means there was an error, so the value of "src" would be either invalid or nil, so putting the "defer" before the err check would possibly result in a panic. I'd say the original code is correct.

Fannie Zhang

unread,
Nov 25, 2021, 3:20:05 AM11/25/21
to Kurtis Rader, golang-nuts

OK, I see. Thank you.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Roland Müller

unread,
Nov 25, 2021, 2:40:36 PM11/25/21
to golan...@googlegroups.com

Hello,

actually trying this with os.Open() the program behaves the same regardless whether the defer is before or after the error handling, Thus, no panic :-)

Isn't anything declared with defer always running after the user code in the given func ends? I am too tired now to look this up.

BR,

Roland

package main

import (
    "fmt"
    "os"
)

func main() {
    f, err := os.Open("/tmp/dat")
    //defer f.Close()
    fmt.Printf("%v\n", f)
    if err != nil {
      fmt.Println("NO FILE")
      //return
   }
   defer f.Close()
   fmt.Println("END")
}

Output:

<nil>
NO FILE
END
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. --
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

David Karr

unread,
Nov 25, 2021, 2:44:25 PM11/25/21
to Roland Müller, golang-nuts
And did you test that with a file path that would fail? 

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/4d0b379f-c2a0-a314-e2d3-5fec36528845%40gmail.com.

Roland Müller

unread,
Nov 25, 2021, 3:49:57 PM11/25/21
to David Karr, golang-nuts
Yes. Actually I only tested with a non-existing file. 

But if you mean that case were also the directory is missing: behavior is still the same.

BR,
Roland

func main() {
    f, err := os.Open("/tmp/dat_is_net_do/dat")

    //defer f.Close()
    fmt.Printf("%v\n", f)
    if err != nil {
      fmt.Println("NO FILE")
      //return
   }
   defer f.Close()
   fmt.Println("END")
}

David Karr

unread,
Nov 25, 2021, 3:55:25 PM11/25/21
to Roland Müller, golang-nuts
I meant, did you test it with a bad path, and with the defer before the error check?  I believe you implied that this didn't result in a failure. It would have happened on exit from the method, if anything.

Roland Müller

unread,
Nov 26, 2021, 2:16:23 AM11/26/21
to David Karr, golang-nuts
Hello David,

the behavior of File.Close() on nil was a surprise for me too. 

Obviously, even just calling a "methods" of some struct instance that is not initialized succeeds in Go. A panic occurs only when the program tries to derefer nil pointer: 

'func (ev *Event) do()' only uses the pointer but does not derefer it. Thus, no panic.

Here 'func (ev Event) do2()' derefers nil pointer already before call. Panic in main.main()

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x47f812]

goroutine 1 [running]:
main.main()
	/tmp/sandbox1681946220/prog.go:14 +0x112

Here 'func (ev *Event) getS()' derefers the pointer. Panic happens inside the main.getS()
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x47f8d4]

goroutine 1 [running]:
main.(*Event).getS(0x4b2a40)
	/tmp/sandbox1571083836/prog.go:35 +0x14


BR,
Roland

tapi...@gmail.com

unread,
Nov 27, 2021, 2:55:10 AM11/27/21
to golang-nuts
Isn't the code should be:

func CopyFile() {
   ...
   if err != nil {
      return
   }
   defer func() { err = src.Close() }()
   ...
}

Brian Candler

unread,
Nov 27, 2021, 5:02:42 AM11/27/21
to golang-nuts
On Saturday, 27 November 2021 at 07:55:10 UTC tapi...@gmail.com wrote:
Isn't the code should be:

func CopyFile() {
   ...
   if err != nil {
      return
   }
   defer func() { err = src.Close() }()
   ...
}

No.

Note that the signature of this function (from the original blog post) is:

func CopyFile(dstName, srcName string) (written int64, err error) {

Given that, I think that your proposed change is a bad idea. Either:
(1) it may mask the real return error value.  For example, if err was set to a non-nil value, but the Close() is successful, err will be set back to nil
Or:
(2) it will make no difference at all.

Whether (1) or (2) applies depends whether the final return is a "naked" one or not.  In this case it isn't:

return io.Copy(dst, src)

Therefore, the value returned is whatever io.Copy() returns, so setting "err" in the defer statement makes no difference at all.  Your version of the code is more confusing, because it implies that it does something which it doesn't.
 
There is a wider question as to whether you should check the error return from Close() in the first place.  In some rare cases it *might* show something, e.g. Close() when you are *writing* a file may return disk full errors for some filesystems.  However it doesn't give a guarantee that all errors are caught.

If you need to guarantee that the data was successfully written to disk then you should call Fsync() and check the return value of that, but that's an expensive operation (it forces the OS to write dirty buffers to disk). Even then there are issues to beware of, especially if multiple threads are calling Fsync() on the same file.  See

Most applications don't need this type of guarantee, i.e. if the disk becomes full or there are I/O errors then they accept that the data on disk will be corrupt.

Axel Wagner

unread,
Nov 27, 2021, 5:21:52 AM11/27/21
to Brian Candler, golang-nuts
On Sat, Nov 27, 2021 at 11:03 AM Brian Candler <b.ca...@pobox.com> wrote:
Whether (1) or (2) applies depends whether the final return is a "naked" one or not.  In this case it isn't:

return io.Copy(dst, src)

Therefore, the value returned is whatever io.Copy() returns


FWIW I think the code in the blog post is bad, as it doesn't check the errors from `Close`. But a) it's an old post, I don't know if it's worth fixing it and b) fixing it might distort the point - teaching the mechanics of defer.

The best way to fix it would probably be to do `return written, dst.Close()` - i.e. call `Close()` twice.
 
There is a wider question as to whether you should check the error return from Close() in the first place.  In some rare cases it *might* show something, e.g. Close() when you are *writing* a file may return disk full errors for some filesystems.  However it doesn't give a guarantee that all errors are caught.

If you need to guarantee that the data was successfully written to disk then you should call Fsync() and check the return value of that, but that's an expensive operation (it forces the OS to write dirty buffers to disk). Even then there are issues to beware of, especially if multiple threads are calling Fsync() on the same file.  See

Most applications don't need this type of guarantee, i.e. if the disk becomes full or there are I/O errors then they accept that the data on disk will be corrupt.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

Brian Candler

unread,
Nov 27, 2021, 5:33:11 AM11/27/21
to golang-nuts
Ergh. Thank you for putting me right.

That is definitely a good reason for avoiding named return values completely.
Reply all
Reply to author
Forward
0 new messages