Details
-
Type:
Bug
-
Status: Resolved
-
Priority:
Major
-
Resolution: Unresolved
-
Affects Version/s: None
-
Fix Version/s: 2.6.0
-
Component/s: SyncGateway
-
Security Level: Public
-
Labels:None
-
Sprint:CBG Sprint 20, CBG Sprint 21
-
Story Points: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.
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