Repository: robertkrimen/otto
Pull request #431 (2022-03-13 21:20:19)

fix example to clean up watchdog goroutine
This fixes the example code for cleaning up long-running VMs.  Previously, they were killed after the timeout: the goroutine tracking this timeout holds a reference to the VM, therefore keeping the entire VM in memory for the duration of the configured timeout regardless of whether it has already completed its execution.

This change adds a second layer of communication to the watchdog goroutine, so that it will terminate itself if the VM has exited within the timeout.  This allows the VM to be garbage collected if no other references to the VM exist.


Patch:
diff --git a/README.markdown b/README.markdown
index 40584d3..5dfa643 100644
--- a/README.markdown
+++ b/README.markdown
@@ -236,12 +236,18 @@ func runUnsafe(unsafe string) {
 
     vm := otto.New()
     vm.Interrupt = make(chan func(), 1) // The buffer prevents blocking
+    watchdogCleanup := make(chan struct{})
+    defer close(watchdogCleanup)
 
     go func() {
-        time.Sleep(2 * time.Second) // Stop after two seconds
-        vm.Interrupt <- func() {
-            panic(halt)
+        select {
+        case <-time.After(2 * time.Second): // Stop after two seconds
+            vm.Interrupt <- func() {
+                panic(halt)
+            }
+        case <-watchdogCleanup:
         }
+        close(vm.Interrupt)
     }()
 
     vm.Run(unsafe) // Here be dragons (risky code)
diff --git a/builtin_array.go b/builtin_array.go
index 98492a3..20e89ee 100644
--- a/builtin_array.go
+++ b/builtin_array.go
@@ -641,7 +641,7 @@ func builtinArray_reduce(call FunctionCall) Value {
 			}
 			for ; index < length; index++ {
 				if key := arrayIndexToString(index); thisObject.hasProperty(key) {
-					accumulator = iterator.call(call.runtime, Value{}, accumulator, thisObject.get(key), key, this)
+					accumulator = iterator.call(call.runtime, Value{}, accumulator, thisObject.get(key), index, this)
 				}
 			}
 			return accumulator
diff --git a/type_go_array.go b/type_go_array.go
index 543cd6b..b9af796 100644
--- a/type_go_array.go
+++ b/type_go_array.go
@@ -62,7 +62,7 @@ func (self _goArrayObject) setValue(index int64, value Value) bool {
 	if !exists {
 		return false
 	}
-	reflectValue, err := value.toReflectValue(reflect.Indirect(self.value).Type().Elem().Kind())
+	reflectValue, err := value.toReflectValue(reflect.Indirect(self.value).Type().Elem())
 	if err != nil {
 		panic(err)
 	}
diff --git a/type_go_map.go b/type_go_map.go
index 7b96c10..88ae40b 100644
--- a/type_go_map.go
+++ b/type_go_map.go
@@ -14,8 +14,8 @@ func (runtime *_runtime) newGoMapObject(value reflect.Value) *_object {
 
 type _goMapObject struct {
 	value     reflect.Value
-	keyKind   reflect.Kind
-	valueKind reflect.Kind
+	keyType   reflect.Type
+	valueType reflect.Type
 }
 
 func _newGoMapObject(value reflect.Value) *_goMapObject {
@@ -24,14 +24,14 @@ func _newGoMapObject(value reflect.Value) *_goMapObject {
 	}
 	self := &_goMapObject{
 		value:     value,
-		keyKind:   value.Type().Key().Kind(),
-		valueKind: value.Type().Elem().Kind(),
+		keyType:   value.Type().Key(),
+		valueType: value.Type().Elem(),
 	}
 	return self
 }
 
 func (self _goMapObject) toKey(name string) reflect.Value {
-	reflectValue, err := stringToReflectValue(name, self.keyKind)
+	reflectValue, err := stringToReflectValue(name, self.keyType.Kind())
 	if err != nil {
 		panic(err)
 	}
@@ -39,7 +39,7 @@ func (self _goMapObject) toKey(name string) reflect.Value {
 }
 
 func (self _goMapObject) toValue(value Value) reflect.Value {
-	reflectValue, err := value.toReflectValue(self.valueKind)
+	reflectValue, err := value.toReflectValue(self.valueType)
 	if err != nil {
 		panic(err)
 	}
diff --git a/type_go_slice.go b/type_go_slice.go
index 58ebe18..6c1253a 100644
--- a/type_go_slice.go
+++ b/type_go_slice.go
@@ -53,7 +53,7 @@ func (self *_goSliceObject) setLength(value Value) {
 }
 
 func (self *_goSliceObject) setValue(index int64, value Value) bool {
-	reflectValue, err := value.toReflectValue(self.value.Type().Elem().Kind())
+	reflectValue, err := value.toReflectValue(self.value.Type().Elem())
 	if err != nil {
 		panic(err)
 	}
diff --git a/value.go b/value.go
index b444227..c55fc95 100644
--- a/value.go
+++ b/value.go
@@ -749,42 +749,13 @@ func (self Value) evaluateBreak(labels []string) _resultKind {
 	return resultReturn
 }
 
-func (self Value) exportNative() interface{} {
-	switch self.kind {
-	case valueUndefined:
-		return self
-	case valueNull:
-		return nil
-	case valueNumber, valueBoolean:
-		return self.value
-	case valueString:
-		switch value := self.value.(type) {
-		case string:
-			return value
-		case []uint16:
-			return string(utf16.Decode(value))
-		}
-	case valueObject:
-		object := self._object()
-		switch value := object.value.(type) {
-		case *_goStructObject:
-			return value.value.Interface()
-		case *_goMapObject:
-			return value.value.Interface()
-		case *_goArrayObject:
-			return value.value.Interface()
-		case *_goSliceObject:
-			return value.value.Interface()
-		}
-	}
-
-	return self
-}
-
 // Make a best effort to return a reflect.Value corresponding to reflect.Kind, but
 // fallback to just returning the Go value we have handy.
-func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
-	if kind != reflect.Float32 && kind != reflect.Float64 && kind != reflect.Interface {
+func (value Value) toReflectValue(typ reflect.Type) (reflect.Value, error) {
+	kind := typ.Kind()
+	switch kind {
+	case reflect.Float32, reflect.Float64, reflect.Interface:
+	default:
 		switch value := value.value.(type) {
 		case float32:
 			_, frac := math.Modf(float64(value))
@@ -801,7 +772,7 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 
 	switch kind {
 	case reflect.Bool: // Bool
-		return reflect.ValueOf(value.bool()), nil
+		return reflect.ValueOf(value.bool()).Convert(typ), nil
 	case reflect.Int: // Int
 		// We convert to float64 here because converting to int64 will not tell us
 		// if a value is outside the range of int64
@@ -809,28 +780,28 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 		if tmp < float_minInt || tmp > float_maxInt {
 			return reflect.Value{}, fmt.Errorf("RangeError: %f (%v) to int", tmp, value)
 		} else {
-			return reflect.ValueOf(int(tmp)), nil
+			return reflect.ValueOf(int(tmp)).Convert(typ), nil
 		}
 	case reflect.Int8: // Int8
 		tmp := value.number().int64
 		if tmp < int64_minInt8 || tmp > int64_maxInt8 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %d (%v) to int8", tmp, value)
 		} else {
-			return reflect.ValueOf(int8(tmp)), nil
+			return reflect.ValueOf(int8(tmp)).Convert(typ), nil
 		}
 	case reflect.Int16: // Int16
 		tmp := value.number().int64
 		if tmp < int64_minInt16 || tmp > int64_maxInt16 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %d (%v) to int16", tmp, value)
 		} else {
-			return reflect.ValueOf(int16(tmp)), nil
+			return reflect.ValueOf(int16(tmp)).Convert(typ), nil
 		}
 	case reflect.Int32: // Int32
 		tmp := value.number().int64
 		if tmp < int64_minInt32 || tmp > int64_maxInt32 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %d (%v) to int32", tmp, value)
 		} else {
-			return reflect.ValueOf(int32(tmp)), nil
+			return reflect.ValueOf(int32(tmp)).Convert(typ), nil
 		}
 	case reflect.Int64: // Int64
 		// We convert to float64 here because converting to int64 will not tell us
@@ -839,7 +810,7 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 		if tmp < float_minInt64 || tmp > float_maxInt64 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %f (%v) to int", tmp, value)
 		} else {
-			return reflect.ValueOf(int64(tmp)), nil
+			return reflect.ValueOf(int64(tmp)).Convert(typ), nil
 		}
 	case reflect.Uint: // Uint
 		// We convert to float64 here because converting to int64 will not tell us
@@ -848,28 +819,28 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 		if tmp < 0 || tmp > float_maxUint {
 			return reflect.Value{}, fmt.Errorf("RangeError: %f (%v) to uint", tmp, value)
 		} else {
-			return reflect.ValueOf(uint(tmp)), nil
+			return reflect.ValueOf(uint(tmp)).Convert(typ), nil
 		}
 	case reflect.Uint8: // Uint8
 		tmp := value.number().int64
 		if tmp < 0 || tmp > int64_maxUint8 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %d (%v) to uint8", tmp, value)
 		} else {
-			return reflect.ValueOf(uint8(tmp)), nil
+			return reflect.ValueOf(uint8(tmp)).Convert(typ), nil
 		}
 	case reflect.Uint16: // Uint16
 		tmp := value.number().int64
 		if tmp < 0 || tmp > int64_maxUint16 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %d (%v) to uint16", tmp, value)
 		} else {
-			return reflect.ValueOf(uint16(tmp)), nil
+			return reflect.ValueOf(uint16(tmp)).Convert(typ), nil
 		}
 	case reflect.Uint32: // Uint32
 		tmp := value.number().int64
 		if tmp < 0 || tmp > int64_maxUint32 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %d (%v) to uint32", tmp, value)
 		} else {
-			return reflect.ValueOf(uint32(tmp)), nil
+			return reflect.ValueOf(uint32(tmp)).Convert(typ), nil
 		}
 	case reflect.Uint64: // Uint64
 		// We convert to float64 here because converting to int64 will not tell us
@@ -878,7 +849,7 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 		if tmp < 0 || tmp > float_maxUint64 {
 			return reflect.Value{}, fmt.Errorf("RangeError: %f (%v) to uint64", tmp, value)
 		} else {
-			return reflect.ValueOf(uint64(tmp)), nil
+			return reflect.ValueOf(uint64(tmp)).Convert(typ), nil
 		}
 	case reflect.Float32: // Float32
 		tmp := value.float64()
@@ -889,13 +860,13 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 		if tmp1 > 0 && (tmp1 < math.SmallestNonzeroFloat32 || tmp1 > math.MaxFloat32) {
 			return reflect.Value{}, fmt.Errorf("RangeError: %f (%v) to float32", tmp, value)
 		} else {
-			return reflect.ValueOf(float32(tmp)), nil
+			return reflect.ValueOf(float32(tmp)).Convert(typ), nil
 		}
 	case reflect.Float64: // Float64
 		value := value.float64()
-		return reflect.ValueOf(float64(value)), nil
+		return reflect.ValueOf(float64(value)).Convert(typ), nil
 	case reflect.String: // String
-		return reflect.ValueOf(value.string()), nil
+		return reflect.ValueOf(value.string()).Convert(typ), nil
 	case reflect.Invalid: // Invalid
 	case reflect.Complex64: // FIXME? Complex64
 	case reflect.Complex128: // FIXME? Complex128
@@ -917,7 +888,11 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 			case *_goSliceObject: // Slice
 				return reflect.ValueOf(vl.value.Interface()), nil
 			}
-			return reflect.ValueOf(value.exportNative()), nil
+			exported := reflect.ValueOf(value.export())
+			if exported.Type().ConvertibleTo(typ) {
+				return exported.Convert(typ), nil
+			}
+			return reflect.Value{}, fmt.Errorf("TypeError: could not convert %v to reflect.Type: %v", exported, typ)
 		case valueEmpty, valueResult, valueReference:
 			// These are invalid, and should panic
 		default:
@@ -926,7 +901,7 @@ func (value Value) toReflectValue(kind reflect.Kind) (reflect.Value, error) {
 	}
 
 	// FIXME Should this end up as a TypeError?
-	panic(fmt.Errorf("invalid conversion of %v (%v) to reflect.Kind: %v", value.kind, value, kind))
+	panic(fmt.Errorf("invalid conversion of %v (%v) to reflect.Type: %v", value.kind, value, typ))
 }
 
 func stringToReflectValue(value string, kind reflect.Kind) (reflect.Value, error) {


Test patch:
diff --git a/issue_test.go b/issue_test.go
index cc6d5ea..783dedc 100644
--- a/issue_test.go
+++ b/issue_test.go
@@ -3,6 +3,7 @@ package otto
 import (
 	"database/sql"
 	"database/sql/driver"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"testing"
@@ -1074,3 +1075,50 @@ func Test_issue317(t *testing.T) {
 		})
 	}
 }
+
+func Test_issue252(t *testing.T) {
+	in := map[string]interface{}{
+		"attr": []interface{}{"string"},
+	}
+	expected := map[string]interface{}{
+		"attr": []string{"changed"},
+	}
+
+	vm := New()
+	err := vm.Set("In", in)
+	require.NoError(t, err)
+
+	result, err := vm.Run(`(function fn() {
+		var tmp = In;
+		tmp.attr = ["changed"];
+		return tmp;
+	})()`)
+	require.NoError(t, err)
+
+	actual, err := result.Export()
+	require.NoError(t, err)
+	require.Equal(t, expected, actual)
+
+	expBs, err := json.Marshal(expected)
+	require.NoError(t, err)
+	actBs, err := json.Marshal(actual)
+	require.NoError(t, err)
+	require.Equal(t, expBs, actBs)
+}
+
+func Test_issue177(t *testing.T) {
+	vm := New()
+	val, err := vm.Run(`
+		var ii = 33;
+		var ret = [3, 2, 1].reduce(
+			function(pv, cv, ci, a) {
+				return pv + cv + ci;
+			}
+		);
+		ret;
+	`)
+	require.NoError(t, err)
+	exp, err := val.Export()
+	require.NoError(t, err)
+	require.Equal(t, float64(9), exp)
+}
diff --git a/reflect_test.go b/reflect_test.go
index 2d3217b..ba5efff 100644
--- a/reflect_test.go
+++ b/reflect_test.go
@@ -8,6 +8,8 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 type _abcStruct struct {
@@ -138,14 +140,16 @@ func (mno _mnoStruct) Func() string {
 }
 
 func TestReflect(t *testing.T) {
-	if true {
-		return
-	}
 	tt(t, func() {
 		// Testing dbgf
 		// These should panic
-		toValue("Xyzzy").toReflectValue(reflect.Ptr)
-		stringToReflectValue("Xyzzy", reflect.Ptr)
+		str := "test"
+		require.Panics(t, func() {
+			toValue("Xyzzy").toReflectValue(reflect.ValueOf(&str).Type())
+		})
+		require.Panics(t, func() {
+			stringToReflectValue("Xyzzy", reflect.Ptr)
+		})
 	})
 }
 
@@ -708,7 +712,7 @@ func Test_reflectMapInterface(t *testing.T) {
             `, "Nothing happens.,1,[object Object],[object Object]")
 
 			is(abc["xyz"], "pqr")
-			is(abc["ghi"], "[object Object]")
+			is(abc["ghi"], map[string]interface{}{})
 			is(abc["jkl"], float64(3.14159))
 			mno, valid := abc["mno"].(*_abcStruct)
 			is(valid, true)
diff --git a/value_test.go b/value_test.go
index 82463f9..8c87733 100644
--- a/value_test.go
+++ b/value_test.go
@@ -307,7 +307,7 @@ func TestExport(t *testing.T) {
 func Test_toReflectValue(t *testing.T) {
 	tt(t, func() {
 		value := toValue(0.0)
-		tmp, err := value.toReflectValue(reflect.Float32)
+		tmp, err := value.toReflectValue(reflect.TypeOf(0.0))
 		is(tmp.Float(), 0.0)
 		is(err, nil)
 	})