From bd547fd0d6ed3535f72a5f6c65996d3e6d394cd3 Mon Sep 17 00:00:00 2001 From: Jim Date: Fri, 2 Oct 2020 10:01:46 -0400 Subject: [PATCH] terminate a canceled session when its last connection is closed. (#520) --- internal/db/migrations/postgres.gen.go | 93 ++++++++++++++++++- .../migrations/postgres/51_connection.up.sql | 93 ++++++++++++++++++- internal/session/repository_session_test.go | 9 +- 3 files changed, 190 insertions(+), 5 deletions(-) diff --git a/internal/db/migrations/postgres.gen.go b/internal/db/migrations/postgres.gen.go index 7b803c897..61f163531 100644 --- a/internal/db/migrations/postgres.gen.go +++ b/internal/db/migrations/postgres.gen.go @@ -4010,7 +4010,7 @@ begin; after insert on session_connection for each row execute procedure insert_new_connection_state(); - -- update_connection_state_on_closed_reason() is used in an update insert trigger on the + -- update_connection_state_on_closed_reason() is used in an update trigger on the -- session_connection table. it will insert a state of "closed" in -- session_connection_state for the closed session connection. create or replace function @@ -4031,6 +4031,9 @@ begin; values (new.public_id, 'closed'); end if; + -- whenever we close a connection, we want to terminate the session if + -- possible. + perform terminate_session_if_possible(new.session_id); end if; return new; end; @@ -4116,6 +4119,94 @@ begin; create trigger insert_session_connection_state before insert on session_connection_state for each row execute procedure insert_session_connection_state(); +-- terminate_session_if_possible takes a session id and terminates the session +-- if the following conditions are met: +-- * the session is expired and all its connections are closed. +-- * the session is canceling and all its connections are closed +-- * the session has exhausted its connection limit and all its connections +-- are closed. +-- +-- Note: this function should align closely with the domain function +-- TerminateCompletedSessions +create or replace function + terminate_session_if_possible(terminate_session_id text) + returns void + as $$ + begin + -- is terminate_session_id in a canceling state + with canceling_session(session_id) as + ( + select + session_id + from + session_state ss + where + ss.session_id = terminate_session_id and + ss.state = 'canceling' and + ss.end_time is null + ) + update session us + set termination_reason = + case + -- timed out sessions + when now() > us.expiration_time then 'timed out' + -- canceling sessions + when us.public_id in( + select + session_id + from + canceling_session cs + where + us.public_id = cs.session_id + ) then 'canceled' + -- default: session connection limit reached. + else 'connection limit' + end + where + -- limit update to just the terminating_session_id + us.public_id = terminate_session_id and + termination_reason is null and + -- session expired or connection limit reached + ( + -- expired sessions... + now() > us.expiration_time or + -- connection limit reached... + ( + select count (*) + from session_connection sc + where + sc.session_id = us.public_id + ) >= connection_limit or + -- canceled sessions + us.public_id in ( + select + session_id + from + canceling_session cs + where + us.public_id = cs.session_id + ) + ) and + -- make sure there are no existing connections + us.public_id not in ( + select + session_id + from + session_connection + where public_id in ( + select + connection_id + from + session_connection_state + where + state != 'closed' and + end_time is null + ) + ); + end; + $$ language plpgsql; + + commit; `), diff --git a/internal/db/migrations/postgres/51_connection.up.sql b/internal/db/migrations/postgres/51_connection.up.sql index b7da8a1d4..9f0b5b241 100644 --- a/internal/db/migrations/postgres/51_connection.up.sql +++ b/internal/db/migrations/postgres/51_connection.up.sql @@ -174,7 +174,7 @@ begin; after insert on session_connection for each row execute procedure insert_new_connection_state(); - -- update_connection_state_on_closed_reason() is used in an update insert trigger on the + -- update_connection_state_on_closed_reason() is used in an update trigger on the -- session_connection table. it will insert a state of "closed" in -- session_connection_state for the closed session connection. create or replace function @@ -195,6 +195,9 @@ begin; values (new.public_id, 'closed'); end if; + -- whenever we close a connection, we want to terminate the session if + -- possible. + perform terminate_session_if_possible(new.session_id); end if; return new; end; @@ -280,4 +283,92 @@ begin; create trigger insert_session_connection_state before insert on session_connection_state for each row execute procedure insert_session_connection_state(); +-- terminate_session_if_possible takes a session id and terminates the session +-- if the following conditions are met: +-- * the session is expired and all its connections are closed. +-- * the session is canceling and all its connections are closed +-- * the session has exhausted its connection limit and all its connections +-- are closed. +-- +-- Note: this function should align closely with the domain function +-- TerminateCompletedSessions +create or replace function + terminate_session_if_possible(terminate_session_id text) + returns void + as $$ + begin + -- is terminate_session_id in a canceling state + with canceling_session(session_id) as + ( + select + session_id + from + session_state ss + where + ss.session_id = terminate_session_id and + ss.state = 'canceling' and + ss.end_time is null + ) + update session us + set termination_reason = + case + -- timed out sessions + when now() > us.expiration_time then 'timed out' + -- canceling sessions + when us.public_id in( + select + session_id + from + canceling_session cs + where + us.public_id = cs.session_id + ) then 'canceled' + -- default: session connection limit reached. + else 'connection limit' + end + where + -- limit update to just the terminating_session_id + us.public_id = terminate_session_id and + termination_reason is null and + -- session expired or connection limit reached + ( + -- expired sessions... + now() > us.expiration_time or + -- connection limit reached... + ( + select count (*) + from session_connection sc + where + sc.session_id = us.public_id + ) >= connection_limit or + -- canceled sessions + us.public_id in ( + select + session_id + from + canceling_session cs + where + us.public_id = cs.session_id + ) + ) and + -- make sure there are no existing connections + us.public_id not in ( + select + session_id + from + session_connection + where public_id in ( + select + connection_id + from + session_connection_state + where + state != 'closed' and + end_time is null + ) + ); + end; + $$ language plpgsql; + + commit; diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index 19b925f33..51ff80335 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -892,13 +892,16 @@ func TestRepository_TerminateCompletedSessions(t *testing.T) { return } assert.NoError(err) - assert.Equal(len(args.wantTermed), got) - + t.Logf("terminated: %d", got) + var foundTerminated int for _, ses := range args.sessions { found, _, err := repo.LookupSession(context.Background(), ses.PublicId) require.NoError(err) _, shouldBeTerminated := args.wantTermed[found.PublicId] if shouldBeTerminated { + if found.TerminationReason != "" { + foundTerminated += 1 + } assert.Equal(args.wantTermed[found.PublicId].String(), found.TerminationReason) t.Logf("terminated %s has a connection limit of %d", found.PublicId, found.ConnectionLimit) conn, err := repo.ListConnections(context.Background(), found.PublicId) @@ -925,7 +928,7 @@ func TestRepository_TerminateCompletedSessions(t *testing.T) { } } } - + assert.Equal(len(args.wantTermed), foundTerminated) }) } }