Uploaded image for project: 'Couchbase Gateway'
  1. Couchbase Gateway
  2. CBG-306

Avoid panics as an error handling mechanism

    XMLWordPrintable

    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.

        Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

          Hide
          adamf Adam Fraser added a comment -

          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

          • channels.SetOf
            • This has usages that aren't following the documented recommendation (only hardcoded known-valid strings).  Instead of removing the panic, may be more appropriate to remove those usages.  Based on an initial review, all usages involve channel names that have already been validated (i.e. they exist on docs).
          • canonicalEncoding
            • This doesn't appear to be a particular risk, as it's only being invoked for structs that have previously been successfully unmarshalled by the same library.  However, switching to error handling and bubbling the error up through createRevID is probably still preferable.
          Show
          adamf Adam Fraser added a comment - 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 channels.SetOf This has usages that aren't following the documented recommendation (only hardcoded known-valid strings).  Instead of removing the panic, may be more appropriate to remove those usages.  Based on an initial review, all usages involve channel names that have already been validated (i.e. they exist on docs). canonicalEncoding This doesn't appear to be a particular risk, as it's only being invoked for structs that have previously been successfully unmarshalled by the same library.  However, switching to error handling and bubbling the error up through createRevID is probably still preferable.
          Hide
          build-team Couchbase Build Team added a comment -

          Build sync_gateway-2.6.0-34 contains sync-gateway-accel commit d379b87 with commit message:
          CBG-306: Avoid panic support (#233)

          Show
          build-team Couchbase Build Team added a comment - Build sync_gateway-2.6.0-34 contains sync-gateway-accel commit d379b87 with commit message: CBG-306 : Avoid panic support (#233)
          Hide
          build-team Couchbase Build Team added a comment -

          Build sync_gateway-2.6.0-34 contains sync-gateway-accel commit d379b87 with commit message:
          CBG-306: Avoid panic support (#233)

          Show
          build-team Couchbase Build Team added a comment - Build sync_gateway-2.6.0-34 contains sync-gateway-accel commit d379b87 with commit message: CBG-306 : Avoid panic support (#233)
          Hide
          build-team Couchbase Build Team added a comment -

          Build sync_gateway-2.6.0-34 contains sync_gateway commit 9f0c074 with commit message:
          CBG-306: Function rename and removal of panic (#4092)

          Show
          build-team Couchbase Build Team added a comment - Build sync_gateway-2.6.0-34 contains sync_gateway commit 9f0c074 with commit message: CBG-306 : Function rename and removal of panic (#4092)
          Hide
          build-team Couchbase Build Team added a comment -

          Build sync_gateway-2.6.0-34 contains sync_gateway commit 9f0c074 with commit message:
          CBG-306: Function rename and removal of panic (#4092)

          Show
          build-team Couchbase Build Team added a comment - Build sync_gateway-2.6.0-34 contains sync_gateway commit 9f0c074 with commit message: CBG-306 : Function rename and removal of panic (#4092)

            People

            Assignee:
            jacques.rascagneres Jacques Rascagneres
            Reporter:
            James Flather James Flather
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:

                Gerrit Reviews

                There are no open Gerrit changes

                  PagerDuty