Skip to content

Commit

Permalink
Recover from potential panic when doing map to JSON serialization (#161)
Browse files Browse the repository at this point in the history
### Why I did it

It is possible that in some edge cases, json.Marshal is unable to serialize map to JSON and panics. I am adding some additional logging at a higher log level and the ability for the function to recover from the panic with a deferred recover function.

#### How I did it

Add deferred recover function when JSON serialization is done and drop the query when unable to provide a JSON to gnmi TypedValue. Add additional logging to give more context of state of map as well as data retrieved from Redis.

#### How to verify it

UT
  • Loading branch information
zbud-msft authored Oct 16, 2023
1 parent 8e13400 commit 07e0b36
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 7 deletions.
20 changes: 19 additions & 1 deletion gnmi_server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3713,6 +3713,24 @@ func TestTableData2MsiUseKey(t *testing.T) {
}
}

func TestRecoverFromJSONSerializationPanic(t *testing.T) {
panicMarshal := func(v interface{}) ([]byte, error) {
panic("json.Marshal panics and is unable to serialize JSON")
}
mock := gomonkey.ApplyFunc(json.Marshal, panicMarshal)
defer mock.Reset()

tblPath := sdc.CreateTablePath("STATE_DB", "NEIGH_STATE_TABLE", "|", "10.0.0.57")
msi := make(map[string]interface{})
sdc.TableData2Msi(&tblPath, true, nil, &msi)

typedValue, err := sdc.Msi2TypedValue(msi)
if typedValue != nil && err != nil {
t.Errorf("Test should recover from panic and have nil TypedValue/Error after attempting JSON serialization")
}

}

func TestGnmiSetBatch(t *testing.T) {
mockCode :=
`
Expand Down Expand Up @@ -4087,4 +4105,4 @@ func init() {

// Inform gNMI server to use redis tcp localhost connection
sdc.UseRedisLocalTcpPort = true
}
}
26 changes: 20 additions & 6 deletions sonic_data_client/db_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ func (c *DbClient) PollRun(q *queue.PriorityQueue, poll chan struct{}, w *sync.W
for gnmiPath, tblPaths := range c.pathG2S {
val, err := tableData2TypedValue(tblPaths, nil)
if err != nil {
log.V(2).Infof("Unable to create gnmi TypedValue due to err: %v", err)
return
}

Expand Down Expand Up @@ -721,6 +722,12 @@ func makeJSON_redis(msi *map[string]interface{}, key *string, op *string, mfv ma
// emitJSON marshalls map[string]interface{} to JSON byte stream.
func emitJSON(v *map[string]interface{}) ([]byte, error) {
//j, err := json.MarshalIndent(*v, "", indentString)
defer func() {
if r := recover(); r != nil {
log.V(2).Infof("Recovered from panic: %v", r)
log.V(2).Infof("Current state of map to be serialized is: %v", *v)
}
}()
j, err := json.Marshal(*v)
if err != nil {
return nil, fmt.Errorf("JSON marshalling error: %v", err)
Expand Down Expand Up @@ -759,9 +766,12 @@ func TableData2Msi(tblPath *tablePath, useKey bool, op *string, msi *map[string]
dbkeys = []string{tblPath.tableName + tblPath.delimitor + tblPath.tableKey}
}

log.V(4).Infof("dbkeys to be pulled from redis %v", dbkeys)

// Asked to use jsonField and jsonTableKey in the final json value
if tblPath.jsonField != "" && tblPath.jsonTableKey != "" {
val, err := redisDb.HGet(dbkeys[0], tblPath.field).Result()
log.V(4).Infof("Data pulled for key %s and field %s: %s", dbkeys[0], tblPath.field, val)
if err != nil {
log.V(3).Infof("redis HGet failed for %v %v", tblPath, err)
// ignore non-existing field which was derived from virtual path
Expand All @@ -779,7 +789,7 @@ func TableData2Msi(tblPath *tablePath, useKey bool, op *string, msi *map[string]
log.V(2).Infof("redis HGetAll failed for %v, dbkey %s", tblPath, dbkey)
return err
}

log.V(4).Infof("Data pulled for dbkey %s: %v", dbkey, fv)
if tblPath.jsonTableKey != "" { // If jsonTableKey was prepared, use it
err = makeJSON_redis(msi, &tblPath.jsonTableKey, op, fv)
} else if (tblPath.tableKey != "" && !useKey) || tblPath.tableName == dbkey {
Expand All @@ -803,12 +813,16 @@ func TableData2Msi(tblPath *tablePath, useKey bool, op *string, msi *map[string]
return nil
}

func msi2TypedValue(msi map[string]interface{}) (*gnmipb.TypedValue, error) {
func Msi2TypedValue(msi map[string]interface{}) (*gnmipb.TypedValue, error) {
log.V(4).Infof("State of map after adding redis data %v", msi)
jv, err := emitJSON(&msi)
if err != nil {
log.V(2).Infof("emitJSON err %s for %v", err, msi)
return nil, fmt.Errorf("emitJSON err %s for %v", err, msi)
}
if jv == nil { // json and err is nil because panic potentially happened
return nil, fmt.Errorf("emitJSON failed to grab json value of map due to potential panic")
}
return &gnmipb.TypedValue{
Value: &gnmipb.TypedValue_JsonIetfVal{
JsonIetfVal: jv,
Expand Down Expand Up @@ -839,20 +853,20 @@ func tableData2TypedValue(tblPaths []tablePath, op *string) (*gnmipb.TypedValue,
log.V(2).Infof("redis HGet failed for %v", tblPath)
return nil, err
}
log.V(4).Infof("Data pulled for key %s and field %s: %s", key, tblPath.field, val)
// TODO: support multiple table paths
return &gnmipb.TypedValue{
Value: &gnmipb.TypedValue_StringVal{
StringVal: val,
}}, nil
}
}

err := TableData2Msi(&tblPath, useKey, nil, &msi)
if err != nil {
return nil, err
}
}
return msi2TypedValue(msi)
return Msi2TypedValue(msi)
}

func enqueueFatalMsg(c *DbClient, msg string) {
Expand Down Expand Up @@ -921,7 +935,7 @@ func dbFieldMultiSubscribe(c *DbClient, gnmiPath *gnmipb.Path, onChange bool, in
}

sendVal := func(msi map[string]interface{}) error {
val, err := msi2TypedValue(msi)
val, err := Msi2TypedValue(msi)
if err != nil {
enqueueFatalMsg(c, err.Error())
return err
Expand Down Expand Up @@ -1178,7 +1192,7 @@ func dbTableKeySubscribe(c *DbClient, gnmiPath *gnmipb.Path, interval time.Durat
sendDeleteField = true
}
delete(msiData, "delete")
val, err := msi2TypedValue(msiData)
val, err := Msi2TypedValue(msiData)
if err != nil {
return err
}
Expand Down

0 comments on commit 07e0b36

Please sign in to comment.