Details
-
Bug
-
Status: Resolved
-
Major
-
Resolution: Unresolved
-
None
-
Security Level: Public
-
None
-
CBG Sprint 20, CBG Sprint 21
-
5
Description
There are a number of places where Sync Gateway might panic:
$ rg 'panic\(' -g '!*test.go' -g '!*testing.go' -c
|
auth/user.go:3
|
auth/role.go:1
|
db/revision_cache.go:2
|
db/revision.go:1
|
db/document.go:1
|
db/revtree.go:3
|
channels/set.go:2
|
base/bucket.go:2
|
channels/change_log.go:1
|
base/logging.go:1
|
base/logger_console.go:1
|
base/util.go:1
|
rest/handler.go:1
|
base/sequence_clock.go:1
|
rest/bulk_api.go:1
|
rest/server_context.go:2
|
service/sg-windows/sg-accel-service/sg-accel-service.go:2
|
service/sg-windows/sg-service/sg-service.go:2
|
Whilst some of these might be legitimate, others should be substituted for error handling and propagation, so that we can isolate a particular failure to the operation/connection/replication that it affects and not bring the whole process down.
A prime example would be in auth.(*userImpl).GetRoles:
func (user *userImpl) GetRoles() []Role {
|
if user.roles == nil {
|
roles := make([]Role, 0, len(user.RolesSince_))
|
for name := range user.RolesSince_ {
|
role, err := user.auth.GetRole(name)
|
//base.Infof(base.KeyAccess, "User %s role %q = %v", base.UD(user.Name_), base.UD(name), base.UD(role))
|
if err != nil {
|
panic(fmt.Sprintf("Error getting user role %q: %v", name, err))
|
} else if role != nil {
|
roles = append(roles, role)
|
}
|
}
|
user.roles = roles
|
}
|
return user.roles
|
}
|
Raising this CBG as a high level ticket to potentially run through each of the panics listed above and comb out those that should be changed.
Attachments
Gerrit Reviews
Activity
Build sync_gateway-2.6.0-34 contains sync-gateway-accel commit d379b87 with commit message:
CBG-306: Avoid panic support (#233)
Build sync_gateway-2.6.0-34 contains sync-gateway-accel commit d379b87 with commit message:
CBG-306: Avoid panic support (#233)
Build sync_gateway-2.6.0-34 contains sync_gateway commit 9f0c074 with commit message:
CBG-306: Function rename and removal of panic (#4092)
Build sync_gateway-2.6.0-34 contains sync_gateway commit 9f0c074 with commit message:
CBG-306: Function rename and removal of panic (#4092)
The case that's been highlighted here (user.GetRoles) appears to be the only one that presents a significant risk (i.e. could be invoked when server returns an error, and isn't recovered in all cases). Given the high priority, have moved that to its own ticket (CBG-308)
The rest are either using panic correctly (identifying that the system is in an unrecoverable state, unrelated to a single operation/connection), or intentionally triggering the recover in the rest handler code. The latter cases could be cleaned up, to avoid unintentional misuse in future (as appears to be the case for user.GetRoles).
There are two other cases that could be consider for a switch to error handling, although they also don't appear to be critical