Repository: seqsense/aws-iot-device-sdk-go
Pull request #126 (2020-07-18 05:18:18)

shadow: avoid document mutation
Deepcopy before returning documents.
Use mutex on update.

~after #125~

at-wat: @kt3k @t-asaka please take a look
codecov[bot]: # [Codecov](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=h1) Report
> Merging [#126](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=desc) into [master](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/commit/86d4f3e5be98c3d4c9eb187d3aa1a2584d488005&el=desc) will **increase** coverage by `0.57%`.
> The diff coverage is `63.46%`.

[![Impacted file tree graph](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126/graphs/tree.svg?width=650&height=150&src=pr&token=St5U6jzMqv)](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   46.11%   46.69%   +0.57%     
==========================================
  Files          24       25       +1     
  Lines        1299     1345      +46     
==========================================
+ Hits          599      628      +29     
- Misses        621      638      +17     
  Partials       79       79              
```


| [Impacted Files](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [shadow/copy.go](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126/diff?src=pr&el=tree#diff-c2hhZG93L2NvcHkuZ28=) | `46.42% <46.42%> (ø)` | |
| [shadow/shadow.go](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126/diff?src=pr&el=tree#diff-c2hhZG93L3NoYWRvdy5nbw==) | `76.08% <83.33%> (+0.89%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=footer). Last update [86d4f3e...513eae5](https://codecov.io/gh/seqsense/aws-iot-device-sdk-go/pull/126?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

kt3k: When I run `example/shadow` command, I saw the following error:

```
> delete
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1348bc6]

goroutine 1 [running]:
github.com/seqsense/aws-iot-device-sdk-go/v4/shadow.(*ThingDocument).clone(0x0, 0x0)
	/Users/kt3k/sq/aws-iot-device-sdk-go/shadow/copy.go:51 +0x36
github.com/seqsense/aws-iot-device-sdk-go/v4/shadow.(*shadow).Document(0xc0001f4690, 0x0)
	/Users/kt3k/sq/aws-iot-device-sdk-go/shadow/shadow.go:383 +0x76
main.main()
	/Users/kt3k/sq/aws-iot-device-sdk-go/examples/shadow/main.go:112 +0xb2c
```
Maybe we need to check nil in `Document()` function of shadow impl?
at-wat: After Delete, thing shadow document doesn't exist and AWS IoT returns error against GetDocument. I think representing it by `nil` document is reasonable.
I have fixed clone of nil document and added some tests.

Patch:
diff --git a/shadow/copy.go b/shadow/copy.go
new file mode 100644
index 0000000..d64237c
--- /dev/null
+++ b/shadow/copy.go
@@ -0,0 +1,59 @@
+// Copyright 2020 SEQSENSE, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package shadow
+
+// cloneState creates deepcopy of thing state.
+func cloneState(s map[string]interface{}) map[string]interface{} {
+	c := map[string]interface{}{}
+	for k, v := range s {
+		switch vv := v.(type) {
+		case map[string]interface{}:
+			c[k] = cloneState(vv)
+		case []interface{}:
+			c[k] = cloneStateSlice(vv)
+		default:
+			c[k] = vv
+		}
+	}
+	return c
+}
+
+// cloneStateSlice creates deepcopy of thing state slice.
+func cloneStateSlice(s []interface{}) []interface{} {
+	c := []interface{}{}
+	for _, v := range s {
+		switch vv := v.(type) {
+		case map[string]interface{}:
+			c = append(c, cloneState(vv))
+		case []interface{}:
+			c = append(c, cloneStateSlice(vv))
+		default:
+			c = append(c, vv)
+		}
+	}
+	return c
+}
+
+// clone returns deepcopy of the document.
+func (s *ThingDocument) clone() *ThingDocument {
+	if s == nil {
+		return nil
+	}
+	c := *s
+	c.State.Desired = cloneState(s.State.Desired)
+	c.State.Reported = cloneState(s.State.Reported)
+	c.State.Delta = cloneState(s.State.Delta)
+	return &c
+}
diff --git a/shadow/shadow.go b/shadow/shadow.go
index c22c288..dfa3c2a 100644
--- a/shadow/shadow.go
+++ b/shadow/shadow.go
@@ -83,10 +83,12 @@ func (s *shadow) getAccepted(msg *mqtt.Message) {
 		s.handleError(err)
 		return
 	}
+	s.mu.Lock()
 	s.doc = doc
+	s.mu.Unlock()
 	s.handleResponse(doc)
 
-	s.handleDelta(s.doc.State.Delta)
+	s.handleDelta(doc.State.Delta)
 }
 
 func (s *shadow) rejected(msg *mqtt.Message) {
@@ -104,7 +106,10 @@ func (s *shadow) updateAccepted(msg *mqtt.Message) {
 		s.handleError(err)
 		return
 	}
-	if err := s.doc.update(doc); err != nil {
+	s.mu.Lock()
+	err := s.doc.update(doc)
+	s.mu.Unlock()
+	if err != nil {
 		s.handleError(err)
 		return
 	}
@@ -117,9 +122,12 @@ func (s *shadow) updateDelta(msg *mqtt.Message) {
 		s.handleError(err)
 		return
 	}
+	s.mu.Lock()
 	ok := s.doc.updateDelta(state)
+	delta := cloneState(s.doc.State.Delta)
+	s.mu.Unlock()
 	if ok {
-		s.handleDelta(s.doc.State.Delta)
+		s.handleDelta(delta)
 	}
 }
 
@@ -129,7 +137,9 @@ func (s *shadow) deleteAccepted(msg *mqtt.Message) {
 		s.handleError(err)
 		return
 	}
+	s.mu.Lock()
 	s.doc = nil
+	s.mu.Unlock()
 	s.handleResponse(doc)
 }
 
@@ -219,7 +229,10 @@ func (s *shadow) Report(ctx context.Context, state interface{}) (*ThingDocument,
 	case res := <-ch:
 		switch r := res.(type) {
 		case *thingDocumentRaw:
-			return s.doc, nil
+			s.mu.Lock()
+			doc := s.doc.clone()
+			s.mu.Unlock()
+			return doc, nil
 		case *ErrorResponse:
 			return nil, r
 		default:
@@ -267,7 +280,10 @@ func (s *shadow) Desire(ctx context.Context, state interface{}) (*ThingDocument,
 	case res := <-ch:
 		switch r := res.(type) {
 		case *thingDocumentRaw:
-			return s.doc, nil
+			s.mu.Lock()
+			doc := s.doc.clone()
+			s.mu.Unlock()
+			return doc, nil
 		case *ErrorResponse:
 			return nil, r
 		default:
@@ -362,7 +378,9 @@ func (s *shadow) Delete(ctx context.Context) error {
 }
 
 func (s *shadow) Document() *ThingDocument {
-	return s.doc
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.doc.clone()
 }
 
 func (s *shadow) OnDelta(cb func(delta map[string]interface{})) {


Test patch:
diff --git a/shadow/copy_test.go b/shadow/copy_test.go
new file mode 100644
index 0000000..b29bb3a
--- /dev/null
+++ b/shadow/copy_test.go
@@ -0,0 +1,103 @@
+// Copyright 2020 SEQSENSE, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package shadow
+
+import (
+	"bytes"
+	"encoding/json"
+	"testing"
+)
+
+func TestClone(t *testing.T) {
+	testCases := map[string]struct {
+		doc    *ThingDocument
+		mutate func(*ThingDocument)
+	}{
+		"Base": {
+			doc: &ThingDocument{Version: 10},
+			mutate: func(doc *ThingDocument) {
+				doc.Version = 1
+			},
+		},
+		"Map": {
+			doc: &ThingDocument{
+				State: ThingState{
+					Desired: map[string]interface{}{
+						"key": "value",
+					},
+				},
+			},
+			mutate: func(doc *ThingDocument) {
+				doc.State.Desired["key"] = "value2"
+			},
+		},
+		"MapInMap": {
+			doc: &ThingDocument{
+				State: ThingState{
+					Desired: map[string]interface{}{
+						"key": map[string]interface{}{
+							"childKey": "value",
+						},
+					},
+				},
+			},
+			mutate: func(doc *ThingDocument) {
+				doc.State.Desired["key"].(map[string]interface{})["childKey"] = "value2"
+			},
+		},
+		"SliceInMap": {
+			doc: &ThingDocument{
+				State: ThingState{
+					Desired: map[string]interface{}{
+						"key": []interface{}{
+							"value1",
+							"value2",
+						},
+					},
+				},
+			},
+			mutate: func(doc *ThingDocument) {
+				doc.State.Desired["key"].([]interface{})[0] = "value1b"
+			},
+		},
+		"Nil": {
+			doc:    nil,
+			mutate: func(doc *ThingDocument) {},
+		},
+	}
+	for name, testCase := range testCases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			originalJSON, err := json.Marshal(testCase.doc)
+			if err != nil {
+				t.Fatal(err)
+			}
+			cloned := testCase.doc.clone()
+			testCase.mutate(cloned)
+
+			resultJSON, err := json.Marshal(testCase.doc)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if !bytes.Equal(originalJSON, resultJSON) {
+				t.Errorf(
+					"Original document is mutated by updating cloned one\nexpected: %s\ngot: %s",
+					originalJSON, resultJSON,
+				)
+			}
+		})
+	}
+}