Skip to content

Cause() behavior has changed #219

Closed
Closed

Description

@freeekanayaka

Hello,

after merging #215, I got some test failures of code that calls errors.Cause(err) where err is returned by some stdlib API: in my case err is returned by net.Conn.Write(), and errors.Cause(err) used to return a net.OpError instance, whereas it now returns a syscall.Errno instance.

I understand the reason for the change, but just wanting to highlight this is introduces backward-incompatible behavior, at least for some consumers.

YMMV re fixing this issue. Other people might find it useful to read, if the stumble upon the same regression.

Activity

  1. aperezg commented on Jan 12, 2020

    @aperezg
    Member

    Could you show a complete example to try to reproduce the behaviour that you say, because the current behaviour in the pkg/errors with method cause is the same unless of the new Wrap way be used.

    Furthermore, You could continue using the version 0.8.1 instead of master or the latest version while you migrate to the new version.

  2. freeekanayaka commented on Jan 12, 2020

    @freeekanayaka
    Author

    Hey,

    if you run this program it will produce different outputs, depending if you use pkg/errors master or v0.8.1:

    package main
    
    import (
    	"fmt"
    	"log"
    	"net"
    
    	"github.com/pkg/errors"
    )
    
    func main() {
    	// Create client/server pair of connections over abstract Unix sockets.
    	listener, err := net.Listen("unix", "")
    	if err != nil {
    		log.Fatalf("net.Listen(): %v", err)
    	}
    	address := listener.Addr().String()
    	client, err := net.Dial("unix", address)
    	if err != nil {
    		log.Fatalf("net.Dial(): %v", err)
    	}
    	server, err := listener.Accept()
    	if err != nil {
    		log.Fatalf("net.Accept(): %v", err)
    	}
    
    	// Close the server end and try to perform a write with the client.
    	server.Close()
    	_, err = client.Write([]byte{1, 2, 3, 4})
    
    	// Wrap the error and then get the cause.
    	err = errors.Wrap(err, "connection failure")
    	fmt.Printf("%T\n", errors.Cause(err))
    }

    In master you'll get:

    $ go run main.go 
    syscall.Errno
    

    while with v0.8.1:

    $ go run main.go 
    *net.OpError
    

    If people use errors.Wrap against stdlib errors, and then look at the type of errors.Cause() then they might be hit by this change.

  3. jayschwa commented on Jan 13, 2020

    @jayschwa
    Contributor

    net.OpError provides an Unwrap method in Go 1.13 that Cause now follows. Maybe #215 should be reverted.

  4. Bak-Jin-Hyeong commented on Jan 13, 2020

    @Bak-Jin-Hyeong

    How about informing users to recommend the use of errors.Is?

    Similar backward compatibility issues will occur when user-defined error types implement interface { Unwrap() error } as well as when they implement interface { Cause() error }.

  5. dcormier commented on Jan 14, 2020

    @dcormier

    This is a frustrating problem. There are places where I'd like to be able to take advantage of the added functionality from the Unwrap method being added in v0.9.0, but this change to Cause breaks too many places.

  6. jayschwa commented on Jan 14, 2020

    @jayschwa
    Contributor

    @aperezg, I recommend reverting PR #215 and tagging v0.9.1. The hypothetical benefit of #215 is not worth the breakage of real, existing code.

  7. aperezg commented on Jan 14, 2020

    @aperezg
    Member

    Perfect so, I will revert this PR and created a new v0.9.1 tag with the old behaviour

  8. aperezg commented on Jan 14, 2020

    @aperezg
    Member

    So the new v0.9.1 is tagged with the previous behaviour https://github.com/pkg/errors/releases/tag/v0.9.1

    Thanks a lot for all information and help :)

  9. puellanivis commented on Jan 15, 2020

    @puellanivis

    Yeah, reverting was the right choice. The errors.Unwrap function does not unwrap an error until it has no unwraps left… it returns a single–iteration of unwrapping.

    There is unfortunately, no way to provide Cause that will not unwrap all the way, while also unwrapping go1.13 errors, without also breaking a bunch of stuff that unwraps not just net errors, but almost certainly *os.PathError as well.

  10. james-johnston-thumbtack commented on Mar 3, 2020

    @james-johnston-thumbtack

    Given that this was reverted, does it make sense to mark the Cause function as deprecated given that it will not generally work properly with Go 1.13 wrapped errors? The replacement would be to use the new Go 1.13 runtime functions in "errors" package for unwrapping and inspecting.

  11. puellanivis commented on Mar 3, 2020

    @puellanivis

    Deprecating Cause is not without merit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions