search

aerospike / aerospike-client-go

Aerospike Client Go
Apache License 2.0
430 stars 198 forks source link

Panics should be replaced with errors #304

Closed chorin1 closed 4 years ago

chorin1 commented 4 years ago

As Dave Cheney once wrote:

panics are always fatal to your program. In panicing you never assume that your caller can solve the problem. Hence panic is only used in exceptional circumstances, ones where it is not possible for your code, or anyone integrating your code to continue.

The current client implementation has numerous functions that may panic (some of which we have experienced). IMO, a database failure or a corrupted record shouldn't obligate the caller to crash. Putting panic recovery aside, in these rare cases, it would be more appropriate to return an error.

khaf commented 4 years ago

What are the panics you experience? The client deliberately panics on some invalid input by design.

chorin1 commented 4 years ago

I have attached below the panic we've received. The issue is more conceptual than this specific panic (even though it would be nice to know what caused it). 

goroutine 1610 [running]:
github.com/aerospike/aerospike-client-go.(*unpacker).unpackBlob(0xc00139b5a8,
0xe9, 0xdc9d01, 0xc0063ad002, 0x0, 0xc00530a900, 0x0)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/unpacker.go:235
+0x589
github.com/aerospike/aerospike-client-go.(*unpacker).unpackObject(0xc00139b5a8,
0x1, 0xc0063ad080, 0xc0063ad080, 0xc0047d1000, 0x4a)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/unpacker.go:324
+0x3cf
github.com/aerospike/aerospike-client-go.(*unpacker).unpackMapNormal(0xc00139b5a8,
0x10, 0x203001, 0x300000002, 0xc000f4d200)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/unpacker.go:141
+0x97
github.com/aerospike/aerospike-client-go.(*unpacker).unpackMap(0xc00139b5a8,
0x10, 0xed8aa0, 0xc00139b530, 0x4304d1, 0x30)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/unpacker.go:134
+0x53
github.com/aerospike/aerospike-client-go.(*unpacker).UnpackMap(0xc00139b5a8,
0xc00a9d5300, 0xc00674c000, 0x7f2e6d9ce008, 0x0)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/unpacker.go:123
+0x6f github.com/aerospike/aerospike-client-go.bytesToParticle(0x13,
0xc00281a000, 0x10000, 0x10000, 0x8, 0x13d0, 0xc64e694617, 0x1796400,
0xc00139b680, 0x566858)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/value.go:1085
+0x548
github.com/aerospike/aerospike-client-go.(*readCommand).parseRecord(0xc0095df600,
0x10c87e0, 0xc0095df600, 0x1, 0x0, 0xccbcb9000000001, 0x0, 0x0, 0xc00139b7a8)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/read_command.go:215
+0x299
github.com/aerospike/aerospike-client-go.(*readCommand).parseResult(0xc0095df600,
0x10c87e0, 0xc0095df600, 0xc007806320, 0x45, 0x0)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/read_command.go:157
+0x744
github.com/aerospike/aerospike-client-go.(*baseCommand).execute(0xc0095df600,
0x10c87e0, 0xc0095df600, 0x1, 0x0, 0x0)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/command.go:1894
+0x7fb github.com/aerospike/aerospike-client-go.(*readCommand).Execute(...)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/read_command.go:255
github.com/aerospike/aerospike-client-go.(*Client).Get(0xc0001d6000, 0x0,
0xc0096dcfc0, 0x0, 0x0, 0x0, 0x0, 0x30000c00139bd18, 0x0)
/var/lib/jenkins/workspace/go/af-installs-endpoint/pkg/mod/github.com/aerospike/aerospike-client-go@v2.9.0+incompatible/client.go:301
+0x1e7
khaf commented 4 years ago

This has been addressed in the latest release. We are open and greatly welcome any further feedback. I'll close this ticket for now, but feel free to reopen or file new issues.