From 4fe38848982abdb544bfb48603e9c24133039707 Mon Sep 17 00:00:00 2001 From: Cris Ward Date: Thu, 27 Jul 2017 15:08:05 +0100 Subject: [PATCH 1/3] Release connection when query cannot be built This was leaking connection when a query could not be build - ie if an invalid column name was used. More info here - https://github.com/crystal-lang/crystal-mysql/issues/40 --- src/db/pool_prepared_statement.cr | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/db/pool_prepared_statement.cr b/src/db/pool_prepared_statement.cr index d50b240..23b049d 100644 --- a/src/db/pool_prepared_statement.cr +++ b/src/db/pool_prepared_statement.cr @@ -34,7 +34,12 @@ module DB clean_connections conn, existing = @db.checkout_some(@connections) @connections << WeakRef.new(conn) unless existing - conn.prepared.build(@query) + begin + conn.prepared.build(@query) + rescue ex + conn.release + raise ex + end end private def clean_connections From 9b03aa6535e9edf2031a7d56bad8b61892abc9eb Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Thu, 7 Sep 2017 19:21:36 -0300 Subject: [PATCH 2/3] Release connection when unprepared statements can't be built Add specs Fix typo in docs --- spec/database_spec.cr | 22 ++++++++++++++++++++++ spec/dummy_driver.cr | 1 + src/db/pool_prepared_statement.cr | 2 +- src/db/pool_unprepared_statement.cr | 8 +++++++- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/spec/database_spec.cr b/spec/database_spec.cr index 2071d27..51a53a3 100644 --- a/spec/database_spec.cr +++ b/spec/database_spec.cr @@ -149,6 +149,28 @@ describe DB::Database do end end + it "should return connection to the pool if prepared statement is unable to be built" do + connection = uninitialized DB::Connection + with_dummy "dummy://localhost:1027?initial_pool_size=1" do |db| + connection = DummyDriver::DummyConnection.connections.first + expect_raises do + db.prepared.exec("syntax error") + end + db.pool.is_available?(connection).should be_true + end + end + + it "should return connection to the pool if unprepared statement is unable to be built" do + connection = uninitialized DB::Connection + with_dummy "dummy://localhost:1027?initial_pool_size=1" do |db| + connection = DummyDriver::DummyConnection.connections.first + expect_raises do + db.unprepared.exec("syntax error") + end + db.pool.is_available?(connection).should be_true + end + end + describe "prepared_statements connection option" do it "defaults to true" do with_dummy "dummy://localhost:1027" do |db| diff --git a/spec/dummy_driver.cr b/spec/dummy_driver.cr index cda056e..b0dcf42 100644 --- a/spec/dummy_driver.cr +++ b/spec/dummy_driver.cr @@ -99,6 +99,7 @@ class DummyDriver < DB::Driver def initialize(connection, @query : String, @prepared : Bool) @params = Hash(Int32 | String, DB::Any).new super(connection) + raise query if query == "syntax error" end protected def perform_query(args : Enumerable) diff --git a/src/db/pool_prepared_statement.cr b/src/db/pool_prepared_statement.cr index 23b049d..e3241e5 100644 --- a/src/db/pool_prepared_statement.cr +++ b/src/db/pool_prepared_statement.cr @@ -29,7 +29,7 @@ module DB end # builds a statement over a real connection - # the conneciton is registered in `@connections` + # the connection is registered in `@connections` private def build_statement clean_connections conn, existing = @db.checkout_some(@connections) diff --git a/src/db/pool_unprepared_statement.cr b/src/db/pool_unprepared_statement.cr index 1a120e5..b597e6a 100644 --- a/src/db/pool_unprepared_statement.cr +++ b/src/db/pool_unprepared_statement.cr @@ -15,7 +15,13 @@ module DB # builds a statement over a real connection private def build_statement - @db.pool.checkout.unprepared.build(@query) + conn = @db.pool.checkout + begin + conn.unprepared.build(@query) + rescue ex + conn.release + raise ex + end end end end From 966b5ac42b1efd4dd03225d91c6e83ef35218f45 Mon Sep 17 00:00:00 2001 From: "Brian J. Cardiff" Date: Thu, 7 Sep 2017 19:29:32 -0300 Subject: [PATCH 3/3] Avoid tracking the connection if the statement failed to be built --- src/db/pool_prepared_statement.cr | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/db/pool_prepared_statement.cr b/src/db/pool_prepared_statement.cr index e3241e5..5a65910 100644 --- a/src/db/pool_prepared_statement.cr +++ b/src/db/pool_prepared_statement.cr @@ -33,13 +33,14 @@ module DB private def build_statement clean_connections conn, existing = @db.checkout_some(@connections) - @connections << WeakRef.new(conn) unless existing begin - conn.prepared.build(@query) + stmt = conn.prepared.build(@query) rescue ex conn.release raise ex end + @connections << WeakRef.new(conn) unless existing + stmt end private def clean_connections