Repository: snowflakedb/gosnowflake
Pull request #354 (2020-12-01 23:10:08)

SNOW-209092 Skip Session Close
### Description
Added a control flow to keep client session alive if parameter enabled.

### Checklist
- [x] Code compiles correctly
- [x] Run ``make fmt`` to fix inconsistent formats
- [x] Run ``make lint`` to get lint errors and fix all of them
- [ ] Created tests which fail without the change (if possible)
- [x] All tests passing
- [ ] Extended the README / documentation, if necessary


sfc-gh-anezvigin: My concern with using `isClientSessionKeepAliveEnabled()` is that if you turn on keep-alive, there's no way to close the session even if you wanted to.
sfc-gh-anezvigin: Also this is a pretty big behavior change for existing users of the driver that are turning off keep alive. Their sessions will stay open on the GS side.
sfc-gh-jbahk: I was thinking that users already enable the `client_session_keep_alive` to have the session persist and turn it off, followed by Close() to properly close the session. What do you think?
sfc-gh-cshi: Seems we don't have end to end test for this session persistence change for go driver. Should we have one?
sfc-gh-anezvigin: > I was thinking that users already enable the client_session_keep_alive to have the session persist and turn it off, followed by Close() to properly close the session. What do you think?

Problem is that Close() does more than just optionally close the snowflake session. Go's SQL interface uses it to clean up resources (eg: open channels, etc) that it uses to manage the query lifecycle. The user must call Close() when using the driver in long running processes.

So the following scenario would create a resource leak:

- Long running process (eg: API server)
- Sets client_session_keep_alive to false
- Does _not_ call Close() on the sql.DB object

Patch:
diff --git a/connection.go b/connection.go
index a5f3a86..0c5a4ef 100644
--- a/connection.go
+++ b/connection.go
@@ -218,9 +218,11 @@ func (sc *snowflakeConn) Close() (err error) {
 	logger.WithContext(sc.ctx).Infoln("Close")
 	sc.stopHeartBeat()
 
-	err = sc.rest.FuncCloseSession(sc.ctx, sc.rest, sc.rest.RequestTimeout)
-	if err != nil {
-		logger.Error(err)
+	if !sc.cfg.KeepSessionAlive {
+		err = sc.rest.FuncCloseSession(sc.ctx, sc.rest, sc.rest.RequestTimeout)
+		if err != nil {
+			logger.Error(err)
+		}
 	}
 	sc.cleanup()
 	return nil
diff --git a/dsn.go b/dsn.go
index 758605d..100ed72 100644
--- a/dsn.go
+++ b/dsn.go
@@ -71,8 +71,9 @@ type Config struct {
 	InsecureMode bool             // driver doesn't check certificate revocation status
 	OCSPFailOpen OCSPFailOpenMode // OCSP Fail Open
 
-	Token         string        // Token to use for OAuth other forms of token based auth
-	TokenAccessor TokenAccessor // Optional token accessor to use
+	Token            string        // Token to use for OAuth other forms of token based auth
+	TokenAccessor    TokenAccessor // Optional token accessor to use
+	KeepSessionAlive bool          // Enables the session to persist even after the connection is closed
 
 	PrivateKey *rsa.PrivateKey // Private key used to sign JWT
 


Test patch:
diff --git a/connection_test.go b/connection_test.go
index 74dd7b4..cd6c3ca 100644
--- a/connection_test.go
+++ b/connection_test.go
@@ -171,7 +171,10 @@ func TestServiceName(t *testing.T) {
 	}
 }
 
+var closedSessionCount = 0
+
 func closeSessionMock(_ context.Context, _ *snowflakeRestful, _ time.Duration) error {
+	closedSessionCount++
 	return &SnowflakeError{
 		Number: ErrSessionGone,
 	}
@@ -191,7 +194,24 @@ func TestCloseIgnoreSessionGone(t *testing.T) {
 	}
 }
 
-// test
+func TestClientSessionPersist(t *testing.T) {
+	sr := &snowflakeRestful{
+		FuncCloseSession: closeSessionMock,
+	}
+	sc := &snowflakeConn{
+		cfg:  &Config{Params: map[string]*string{}},
+		rest: sr,
+	}
+	sc.cfg.KeepSessionAlive = true
+	count := closedSessionCount
+	if sc.Close() != nil {
+		t.Error("Connection close should not return error")
+	}
+	if count != closedSessionCount {
+		t.Fatal("close session was called")
+	}
+}
+
 func TestFetchResultByQueryID(t *testing.T) {
 	fetchResultByQueryID(t, nil, nil)
 }