Browse Source

codec: implicitly call Close() by default

We introduced a Close() method to Encoder and Decoder.
This gives users a way to explicitly close and returned shared resources
- [N]byte buffers used by bufioEncWriter and bufioDecReader
- large structures needed for naked decoding effectively
- structures needed for mapping types to encode/decode functions

However, forcing users to explicitly call Close() is unexpected, and
should not be the default.

We now have a flag: DoNotClose (default=false). If set to true, then
we will not Close() after a Encode/Decode call.

As extensions can call Encode/Decode within a corresponding Encode/Decode
call, we use a counter to increment each time Encode/Decode is called.
When that counter resets to 0, we will call Close() unless DoNotClose=true.

In addition, we ensure that tests have DoNotClose=true configured, and we
handle explicitly calling Close() within the tests.
Ugorji Nwoke 7 years ago
parent
commit
856da096db
5 changed files with 55 additions and 9 deletions
  1. 3 3
      codec/build.sh
  2. 13 1
      codec/decode.go
  3. 12 1
      codec/encode.go
  4. 19 2
      codec/helper.go
  5. 8 2
      codec/shared_test.go

+ 3 - 3
codec/build.sh

@@ -178,7 +178,7 @@ _clean() {
 _usage() {
 _usage() {
     cat <<EOF
     cat <<EOF
 primary usage: $0 
 primary usage: $0 
-    -[tmpfxnle]           -> [tests, make, prebuild (force) (external), inlining diagnostics, mid-stack inlining, race detector]
+    -[tmpfxnld]           -> [tests, make, prebuild (force) (external), inlining diagnostics, mid-stack inlining, race detector]
     -v                    -> verbose
     -v                    -> verbose
 EOF
 EOF
     if [[ "$(type -t _usage_run)" = "function" ]]; then _usage_run ; fi
     if [[ "$(type -t _usage_run)" = "function" ]]; then _usage_run ; fi
@@ -191,7 +191,7 @@ _main() {
     zargs=()
     zargs=()
     zbenchflags=""
     zbenchflags=""
     OPTIND=1
     OPTIND=1
-    while getopts ":ctmnrgupfvxlzeb:" flag
+    while getopts ":ctmnrgupfvxlzdb:" flag
     do
     do
         case "x$flag" in
         case "x$flag" in
             'xf') zforce=1 ;;
             'xf') zforce=1 ;;
@@ -199,7 +199,7 @@ _main() {
             'xv') zverbose=1 ;;
             'xv') zverbose=1 ;;
             'xl') zargs+=("-gcflags"); zargs+=("-l=4") ;;
             'xl') zargs+=("-gcflags"); zargs+=("-l=4") ;;
             'xn') zargs+=("-gcflags"); zargs+=("-m=2") ;;
             'xn') zargs+=("-gcflags"); zargs+=("-m=2") ;;
-            'xe') zargs+=("-race") ;;
+            'xd') zargs+=("-race") ;;
             'xb') x='b'; zbenchflags=${OPTARG} ;;
             'xb') x='b'; zbenchflags=${OPTARG} ;;
             x\?) _usage; return 1 ;;
             x\?) _usage; return 1 ;;
             *) x=$flag ;;
             *) x=$flag ;;

+ 13 - 1
codec/decode.go

@@ -2363,7 +2363,13 @@ type Decoder struct {
 
 
 	depth    int16
 	depth    int16
 	maxdepth int16
 	maxdepth int16
-	_        [4]uint8 // padding
+
+	// Extensions can call Decode() within a current Decode() call.
+	// We need to know when the top level Decode() call returns,
+	// so we can decide whether to Close() or not.
+	calls uint16 // what depth in mustDecode are we in now.
+
+	_ [2]uint8 // padding
 
 
 	is map[string]string // used for interning strings
 	is map[string]string // used for interning strings
 
 
@@ -2424,6 +2430,7 @@ func (d *Decoder) resetCommon() {
 	d.n.reset()
 	d.n.reset()
 	d.d.reset()
 	d.d.reset()
 	d.err = nil
 	d.err = nil
+	d.calls = 0
 	d.depth = 0
 	d.depth = 0
 	d.maxdepth = d.h.MaxDepth
 	d.maxdepth = d.h.MaxDepth
 	if d.maxdepth <= 0 {
 	if d.maxdepth <= 0 {
@@ -2583,12 +2590,17 @@ func (d *Decoder) MustDecode(v interface{}) {
 // This provides insight to the code location that triggered the error.
 // This provides insight to the code location that triggered the error.
 func (d *Decoder) mustDecode(v interface{}) {
 func (d *Decoder) mustDecode(v interface{}) {
 	// TODO: Top-level: ensure that v is a pointer and not nil.
 	// TODO: Top-level: ensure that v is a pointer and not nil.
+	d.calls++
 	if d.d.TryDecodeAsNil() {
 	if d.d.TryDecodeAsNil() {
 		setZero(v)
 		setZero(v)
 	} else {
 	} else {
 		d.decode(v)
 		d.decode(v)
 	}
 	}
 	// xprintf(">>>>>>>> >>>>>>>> num decFns: %v\n", d.cf.sn)
 	// xprintf(">>>>>>>> >>>>>>>> num decFns: %v\n", d.cf.sn)
+	d.calls--
+	if !d.h.DoNotClose && d.calls == 0 {
+		d.Close()
+	}
 }
 }
 
 
 // func (d *Decoder) deferred(err1 *error) {
 // func (d *Decoder) deferred(err1 *error) {

+ 12 - 1
codec/encode.go

@@ -1261,7 +1261,12 @@ type Encoder struct {
 	ci set
 	ci set
 	codecFnPooler
 	codecFnPooler
 
 
-	b [3 * 8]byte // for encoding chan or (non-addressable) [N]byte
+	// Extensions can call Encode() within a current Encode() call.
+	// We need to know when the top level Encode() call returns,
+	// so we can decide whether to Close() or not.
+	calls uint16 // what depth in mustEncode are we in now.
+
+	b [(3 * 8) - 2]byte // for encoding chan or (non-addressable) [N]byte
 
 
 	// ---- writable fields during execution --- *try* to keep in sep cache line
 	// ---- writable fields during execution --- *try* to keep in sep cache line
 
 
@@ -1317,6 +1322,7 @@ func (e *Encoder) resetCommon() {
 	_, e.js = e.hh.(*JsonHandle)
 	_, e.js = e.hh.(*JsonHandle)
 	e.e.reset()
 	e.e.reset()
 	e.err = nil
 	e.err = nil
+	e.calls = 0
 }
 }
 
 
 // Reset resets the Encoder with a new output stream.
 // Reset resets the Encoder with a new output stream.
@@ -1494,9 +1500,14 @@ func (e *Encoder) MustEncode(v interface{}) {
 }
 }
 
 
 func (e *Encoder) mustEncode(v interface{}) {
 func (e *Encoder) mustEncode(v interface{}) {
+	e.calls++
 	e.encode(v)
 	e.encode(v)
 	e.e.atEndOfEncode()
 	e.e.atEndOfEncode()
 	e.w.end()
 	e.w.end()
+	e.calls--
+	if !e.h.DoNotClose && e.calls == 0 {
+		e.Close()
+	}
 }
 }
 
 
 // func (e *Encoder) deferred(err1 *error) {
 // func (e *Encoder) deferred(err1 *error) {

+ 19 - 2
codec/helper.go

@@ -144,8 +144,8 @@ const (
 	// Note that calling SetFinalizer is always expensive,
 	// Note that calling SetFinalizer is always expensive,
 	// as code must be run on the systemstack even for SetFinalizer(t, nil).
 	// as code must be run on the systemstack even for SetFinalizer(t, nil).
 	//
 	//
-	// We document that folks SHOULD call Close() when done, or can explicitly
-	// call SetFinalizer themselves e.g.
+	// We document that folks SHOULD call Close() when done, or they can
+	// explicitly call SetFinalizer themselves e.g.
 	//    runtime.SetFinalizer(e, (*Encoder).Close)
 	//    runtime.SetFinalizer(e, (*Encoder).Close)
 	//    runtime.SetFinalizer(d, (*Decoder).Close)
 	//    runtime.SetFinalizer(d, (*Decoder).Close)
 	useFinalizers          = false
 	useFinalizers          = false
@@ -536,6 +536,23 @@ type BasicHandle struct {
 	// (for Cbor and Msgpack), where time.Time was not a builtin supported type.
 	// (for Cbor and Msgpack), where time.Time was not a builtin supported type.
 	TimeNotBuiltin bool
 	TimeNotBuiltin bool
 
 
+	// DoNotClose configures whether Close() is implicitly called after an encode or
+	// decode call.
+	//
+	// If you will hold onto an Encoder or Decoder for re-use, by calling Reset(...)
+	// on it, then you do not want it to be implicitly closed after each Encode/Decode call.
+	// Doing so will unnecessarily return resources to the shared pool, only for you to
+	// grab them right after again to do another Encode/Decode call.
+	//
+	// Instead, you configure DoNotClose=true, and you explicitly call Close() when
+	// you are truly done.
+	//
+	// As an alternative, you can explicitly set a finalizer - so its resources
+	// are returned to the shared pool before it is garbage-collected. Do it as below:
+	//    runtime.SetFinalizer(e, (*Encoder).Close)
+	//    runtime.SetFinalizer(d, (*Decoder).Close)
+	DoNotClose bool
+
 	// ---- cache line
 	// ---- cache line
 
 
 	DecodeOptions
 	DecodeOptions

+ 8 - 2
codec/shared_test.go

@@ -139,8 +139,14 @@ func init() {
 	testHEDs = make([]testHED, 0, 32)
 	testHEDs = make([]testHED, 0, 32)
 	testHandles = append(testHandles,
 	testHandles = append(testHandles,
 		// testNoopH,
 		// testNoopH,
-		testMsgpackH, testBincH, testSimpleH,
-		testCborH, testJsonH)
+		testMsgpackH, testBincH, testSimpleH, testCborH, testJsonH)
+	// set DoNotClose on each handle
+	testMsgpackH.DoNotClose = true
+	testBincH.DoNotClose = true
+	testSimpleH.DoNotClose = true
+	testCborH.DoNotClose = true
+	testJsonH.DoNotClose = true
+
 	testInitFlags()
 	testInitFlags()
 	benchInitFlags()
 	benchInitFlags()
 }
 }