fix(Data::ODBC): use connection and login timeouts in ODBC session implementation (#4721)

* fix(Data::ODBC): use connection and login timeouts in ODBC session implementation (#4366)

* fix(Data::ODBC): use only connection timeout in ODBC session implementation (#4366)

* fix(ODBC): consolidate login timeout; create temp directory if it doesn't exist #4366

---------

Co-authored-by: Alex Fabijanic <alex@pocoproject.org>

Data/DataTest/src/SQLExecutor.cpp

@@ -9,6 +9,7 @@
 
 
 #include "CppUnit/TestCase.h"
+#include "CppUnit/CppUnitException.h"
 #include "Poco/Data/Test/SQLExecutor.h"
 #include "Poco/String.h"
 #include "Poco/Format.h"
@@ -3833,6 +3834,8 @@ void SQLExecutor::autoCommit()
 
 void SQLExecutor::transactionIsolation()
 {
+	try
+	{
 	auto ti = session().getTransactionIsolation();
 
 	// these are just calls to check the transactional capabilities of the session
@@ -3843,6 +3846,15 @@ void SQLExecutor::transactionIsolation()
 	setTransactionIsolation(session(), Session::TRANSACTION_READ_COMMITTED);
 
 	setTransactionIsolation(session(), ti);
+	}
+	catch(const Poco::Exception& ex)
+	{
+		std::cerr << ex.displayText() << std::endl;
+	}
+	catch(const std::exception& ex)
+	{
+		std::cerr << ex.what() << std::endl;
+	}
 }
 
 
@@ -4051,117 +4063,135 @@ void SQLExecutor::transaction(const std::string& connector, const std::string& c
 	else if (local.hasTransactionIsolation(Session::TRANSACTION_READ_COMMITTED))
 		setTransactionIsolation(local, Session::TRANSACTION_READ_COMMITTED);
 
-	std::string tableName("Person");
-	std::vector<std::string> lastNames = {"LN1", "LN2"};
-	std::vector<std::string> firstNames = {"FN1", "FN2"};
-	std::vector<std::string> addresses = {"ADDR1", "ADDR2"};
-	std::vector<int> ages = {1, 2};
-	int count = 0, locCount = 0;
-	std::string result;
-
-	session().setFeature("autoCommit", true);
-	assertTrue (!session().isTransaction());
-	session().setFeature("autoCommit", false);
-	assertTrue (!session().isTransaction());
-	session().setTransactionIsolation(Session::TRANSACTION_READ_COMMITTED);
-
-	{
-		Transaction trans(session());
-		assertTrue (trans.isActive());
-		assertTrue (session().isTransaction());
-
-		session() << formatSQL("INSERT INTO Person VALUES (?,?,?,?)"), use(lastNames), use(firstNames), use(addresses), use(ages), now;
-
-		assertTrue (session().isTransaction());
-		assertTrue (trans.isActive());
-
-		session() << "SELECT COUNT(*) FROM Person", into(count), now;
-		assertTrue (2 == count);
-		assertTrue (session().isTransaction());
-		assertTrue (trans.isActive());
-	}
-	assertTrue (!session().isTransaction());
-
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (0 == count);
-	assertTrue (!(session().impl()->shouldParse() && session().isTransaction()));
-	session().commit();
-
-	{
-		Transaction trans(session());
-		session() << formatSQL("INSERT INTO Person VALUES (?,?,?,?)"), use(lastNames), use(firstNames), use(addresses), use(ages), now;
-
-		Statement stmt1 = (local << "SELECT COUNT(*) FROM Person", into(locCount), async, now);
-
-		assertTrue (session().isTransaction());
-		assertTrue (trans.isActive());
-		trans.commit();
-		assertTrue (!session().isTransaction());
-		assertTrue (!trans.isActive());
-
-		stmt1.wait();
-		assertTrue (2 == locCount);
-	}
-
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (2 == count);
-
-	session() << "DELETE FROM Person", now;
-
-	Statement stmt1 = (local << "SELECT count(*) FROM Person", into(locCount), async, now);
-
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (0 == count);
 	try
 	{
-		stmt1.wait(5000);
+		std::string tableName("Person");
-		if (readUncommitted &&
+		std::vector<std::string> lastNames = {"LN1", "LN2"};
-			local.hasTransactionIsolation(Session::TRANSACTION_READ_UNCOMMITTED) &&
+		std::vector<std::string> firstNames = {"FN1", "FN2"};
-			local.getTransactionIsolation() == Session::TRANSACTION_READ_UNCOMMITTED)
+		std::vector<std::string> addresses = {"ADDR1", "ADDR2"};
-			assertTrue (0 == locCount);
+		std::vector<int> ages = {1, 2};
-	} catch (TimeoutException&)
+		int count = 0, locCount = 0;
-	{ std::cerr << '[' << name() << ']' << " Warning: async query timed out." << std::endl; }
+		std::string result;
-	session().commit();
+
-	// repeat for those that don't support uncommitted read isolation
+		session().setFeature("autoCommit", true);
-	if (local.getTransactionIsolation() == Session::TRANSACTION_READ_COMMITTED)
+		assertTrue (!session().isTransaction());
-	{
+		session().setFeature("autoCommit", false);
-		stmt1.wait();
+		assertTrue (!session().isTransaction());
-		local << "SELECT count(*) FROM Person", into(locCount), now;
+		session().setTransactionIsolation(Session::TRANSACTION_READ_COMMITTED);
-		assertTrue (0 == locCount);
+
+		{
+			Transaction trans(session());
+			assertTrue (trans.isActive());
+			assertTrue (session().isTransaction());
+
+			session() << formatSQL("INSERT INTO Person VALUES (?,?,?,?)"), use(lastNames), use(firstNames), use(addresses), use(ages), now;
+
+			assertTrue (session().isTransaction());
+			assertTrue (trans.isActive());
+
+			session() << "SELECT COUNT(*) FROM Person", into(count), now;
+			assertEqual (2, count);
+			assertTrue (session().isTransaction());
+			assertTrue (trans.isActive());
+		}
+		assertTrue (!session().isTransaction());
+
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (0, count);
+		assertTrue (!(session().impl()->shouldParse() && session().isTransaction()));
+		session().commit();
+
+		{
+			Transaction trans(session());
+			session() << formatSQL("INSERT INTO Person VALUES (?,?,?,?)"), use(lastNames), use(firstNames), use(addresses), use(ages), now;
+
+			Statement stmt1 = (local << "SELECT COUNT(*) FROM Person", into(locCount), async, now);
+
+			assertTrue (session().isTransaction());
+			assertTrue (trans.isActive());
+			trans.commit();
+			assertTrue (!session().isTransaction());
+			assertTrue (!trans.isActive());
+
+			stmt1.wait();
+			assertEqual (2, locCount);
+		}
+
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (2, count);
+
+		session() << "DELETE FROM Person", now;
+
+		Statement stmt1 = (local << "SELECT count(*) FROM Person", into(locCount), async, now);
+
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (0, count);
+		try
+		{
+			stmt1.wait(5000);
+			if (readUncommitted &&
+				local.hasTransactionIsolation(Session::TRANSACTION_READ_UNCOMMITTED) &&
+				local.getTransactionIsolation() == Session::TRANSACTION_READ_UNCOMMITTED)
+				assertEqual (0, locCount);
+		}
+		catch (TimeoutException&)
+		{
+			std::cerr << '[' << name() << ']' << " Warning: async query timed out." << std::endl;
+		}
+		catch (CppUnit::CppUnitException& ex)
+		{
+			std::cerr << " Warning: " << ex.what() << std::endl;
+		}
+		catch (std::exception& ex)
+		{
+			std::cerr << " Warning: " << ex.what() << std::endl;
+		}
+		session().commit();
+		// repeat for those that don't support uncommitted read isolation
+		if (local.getTransactionIsolation() == Session::TRANSACTION_READ_COMMITTED)
+		{
+			stmt1.wait();
+			local << "SELECT count(*) FROM Person", into(locCount), now;
+			assertEqual (0, locCount);
+		}
+
+		std::string sql1 = format("INSERT INTO Person VALUES ('%s','%s','%s',%d)", lastNames[0], firstNames[0], addresses[0], ages[0]);
+		std::string sql2 = format("INSERT INTO Person VALUES ('%s','%s','%s',%d)", lastNames[1], firstNames[1], addresses[1], ages[1]);
+		std::vector<std::string> sql;
+		sql.push_back(sql1);
+		sql.push_back(sql2);
+
+		Transaction trans(session());
+
+		trans.execute(sql1, false);
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (1, count);
+		trans.execute(sql2, false);
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (2, count);
+
+		Statement stmt2 = (local << "SELECT COUNT(*) FROM Person", into(locCount), async, now);
+
+		trans.rollback();
+
+		stmt2.wait();
+		assertEqual (0, locCount);
+
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (0, count);
+
+		trans.execute(sql);
+
+		Statement stmt3 = (local << "SELECT COUNT(*) FROM Person", into(locCount), now);
+		assertEqual (2, locCount);
+
+		session() << "SELECT count(*) FROM Person", into(count), now;
+		assertEqual (2, count);
+	}
+	catch (std::exception& ex)
+	{
+		std::cerr << " Warning: " << ex.what() << std::endl;
 	}
-
-	std::string sql1 = format("INSERT INTO Person VALUES ('%s','%s','%s',%d)", lastNames[0], firstNames[0], addresses[0], ages[0]);
-	std::string sql2 = format("INSERT INTO Person VALUES ('%s','%s','%s',%d)", lastNames[1], firstNames[1], addresses[1], ages[1]);
-	std::vector<std::string> sql;
-	sql.push_back(sql1);
-	sql.push_back(sql2);
-
-	Transaction trans(session());
-
-	trans.execute(sql1, false);
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (1 == count);
-	trans.execute(sql2, false);
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (2 == count);
-
-	Statement stmt2 = (local << "SELECT COUNT(*) FROM Person", into(locCount), async, now);
-
-	trans.rollback();
-
-	stmt2.wait();
-	assertTrue (0 == locCount);
-
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (0 == count);
-
-	trans.execute(sql);
-
-	Statement stmt3 = (local << "SELECT COUNT(*) FROM Person", into(locCount), now);
-	assertTrue (2 == locCount);
-
-	session() << "SELECT count(*) FROM Person", into(count), now;
-	assertTrue (2 == count);
 	session().commit();
 
 	// restore the original transaction state

Data/MySQL/src/Connector.cpp

@@ -44,9 +44,9 @@ const std::string& Connector::name() const
 
 
 Poco::AutoPtr<Poco::Data::SessionImpl> Connector::createSession(const std::string& connectionString,
-	std::size_t timeout)
+	std::size_t loginTimeout)
 {
-	return Poco::AutoPtr<Poco::Data::SessionImpl>(new SessionImpl(connectionString, timeout));
+	return Poco::AutoPtr<Poco::Data::SessionImpl>(new SessionImpl(connectionString, loginTimeout));
 }
 
 

Data/ODBC/include/Poco/Data/ODBC/SessionImpl.h

@@ -160,14 +160,6 @@ public:
 	int maxStatementLength() const;
 		/// Returns maximum length of SQL statement allowed by driver.
 
-	void setLoginTimeout(const std::string&, const Poco::Any& value);
-		/// Sets the timeout (in seconds) for the session login.
-		/// Value must be of type (unsigned) int.
-		/// It must be set prior to logging in.
-
-	Poco::Any getLoginTimeout(const std::string&) const;
-		/// Returns the timeout (in seconds) for the session login.
-
 	void setQueryTimeout(const std::string&, const Poco::Any& value);
 		/// Sets the timeout (in seconds) for queries.
 		/// Value must be of type int.
@@ -322,12 +314,6 @@ inline bool SessionImpl::isTransactionIsolation(Poco::UInt32 ti) const
 }
 
 
-inline Poco::Any SessionImpl::getLoginTimeout(const std::string&) const
-{
-	return _db.getLoginTimeout();
-}
-
-
 inline void SessionImpl::setQueryTimeout(const std::string&, const Poco::Any& value)
 {
 	_queryTimeout = Poco::AnyCast<int>(value);

Data/ODBC/src/ConnectionHandle.cpp

@@ -30,7 +30,7 @@ const std::string ConnectionHandle::CANT_SET_ATTR_SQLSTATE = "HY011";
 
 
 ConnectionHandle::ConnectionHandle(const std::string& connectString, SQLULEN loginTimeout, SQLULEN timeout):
-	_pEnvironment(nullptr),
+	_pEnvironment(SQL_NULL_HENV),
 	_hdbc(SQL_NULL_HDBC),
 	_connectString(connectString)
 {
@@ -59,7 +59,7 @@ void ConnectionHandle::alloc()
 	if (Utility::isError(SQLAllocHandle(SQL_HANDLE_DBC, _pEnvironment->handle(), &_hdbc)))
 	{
 		delete _pEnvironment;
-		_pEnvironment = nullptr;
+		_pEnvironment = SQL_NULL_HENV;
 		_hdbc = SQL_NULL_HDBC;
 		throw ODBCException("ODBC: Could not allocate connection handle.");
 	}
@@ -77,7 +77,7 @@ void ConnectionHandle::free()
 	if (_pEnvironment)
 	{
 		delete _pEnvironment;
-		_pEnvironment = 0;
+		_pEnvironment = SQL_NULL_HENV;
 	}
 }
 
@@ -122,8 +122,8 @@ bool ConnectionHandle::connect(const std::string& connectString, SQLULEN loginTi
 
 	setTimeouts(loginTimeout, timeout);
 
-	if (Utility::isError(Poco::Data::ODBC::SQLDriverConnect(_hdbc
+	if (*this && Utility::isError(Poco::Data::ODBC::SQLDriverConnect(_hdbc
-		, NULL
+		, nullptr
 		,(SQLCHAR*) _connectString.c_str()
 		,(SQLSMALLINT) SQL_NTS
 		, connectOutput
@@ -172,7 +172,10 @@ void ConnectionHandle::setTimeoutImpl(SQLULEN timeout, SQLINTEGER attribute)
 	if (attribute != SQL_ATTR_LOGIN_TIMEOUT && attribute != SQL_ATTR_CONNECTION_TIMEOUT)
 		throw InvalidArgumentException(Poco::format("ODBC::ConnectionHandle::setTimeoutImpl(%d)", attribute));
 
-	if (Utility::isError(SQLSetConnectAttr(_hdbc, attribute, (SQLPOINTER) timeout, 0)))
+	if (attribute == SQL_ATTR_CONNECTION_TIMEOUT && !isConnected()) // can't set this on not connected session
+		return;
+
+	if (*this && Utility::isError(SQLSetConnectAttr(_hdbc, attribute, (SQLPOINTER) timeout, 0)))
 	{
 		ConnectionError e(_hdbc);
 		std::string name;
@@ -201,7 +204,7 @@ void ConnectionHandle::setTimeoutImpl(SQLULEN timeout, SQLINTEGER attribute)
 int ConnectionHandle::getTimeoutImpl(SQLINTEGER attribute) const
 {
 	SQLUINTEGER timeout = 0;
-	if (Utility::isError(SQLGetConnectAttr(_hdbc, attribute, &timeout, sizeof(timeout), 0)))
+	if (*this && Utility::isError(SQLGetConnectAttr(_hdbc, attribute, &timeout, sizeof(timeout), nullptr)))
 	{
 		ConnectionError e(_hdbc);
 		if (isUnsupported(e))
@@ -268,7 +271,7 @@ bool ConnectionHandle::isConnected() const
 		SQL_ATTR_CONNECTION_DEAD,
 		&value,
 		sizeof(value),
-		0))) return false;
+		nullptr))) return false;
 
 	return (SQL_CD_FALSE == value);
 }

Data/ODBC/src/SessionImpl.cpp

@@ -50,6 +50,7 @@ SessionImpl::SessionImpl(const std::string& connect,
 	// https://github.com/MicrosoftDocs/sql-docs/blob/live/docs/odbc/reference/appendixes/using-the-odbc-cursor-library.md
 	setCursorUse("", ODBC_CURSOR_USE_IF_NEEDED);
 
+	_db.setLoginTimeout(loginTimeout);
 	open();
 }
 
@@ -76,6 +77,7 @@ SessionImpl::SessionImpl(const std::string& connect,
 	// https://github.com/MicrosoftDocs/sql-docs/blob/live/docs/odbc/reference/appendixes/using-the-odbc-cursor-library.md
 	setCursorUse("", ODBC_CURSOR_USE_IF_NEEDED);
 
+	_db.setLoginTimeout(getLoginTimeout());
 	open();
 }
 
@@ -129,10 +131,6 @@ void SessionImpl::addFeatures()
 		&SessionImpl::setMaxFieldSize,
 		&SessionImpl::getMaxFieldSize);
 
-	addProperty("loginTimeout",
-		&SessionImpl::setLoginTimeout,
-		&SessionImpl::getLoginTimeout);
-
 	addProperty("queryTimeout",
 		&SessionImpl::setQueryTimeout,
 		&SessionImpl::getQueryTimeout);
@@ -163,7 +161,7 @@ void SessionImpl::open(const std::string& connect)
 	if (connectionString().empty())
 		throw InvalidArgumentException("SessionImpl::open(): Connection string empty");
 
-	if (_db.connect(connectionString()))
+	if (_db.connect(connectionString(), static_cast<SQLULEN>(getLoginTimeout())))
 	{
 		setProperty("handle", _db.handle());
 
@@ -266,22 +264,6 @@ std::size_t SessionImpl::getConnectionTimeout() const
 }
 
 
-void SessionImpl::setLoginTimeout(const std::string&, const Poco::Any& value)
-{
-	int timeout = 0;
-	try
-	{
-		timeout = Poco::AnyCast<int>(value);
-	}
-	catch(const Poco::BadCastException&)
-	{
-		timeout = Poco::AnyCast<unsigned int>(value);
-	}
-
-	_db.setLoginTimeout(timeout);
-}
-
-
 bool SessionImpl::canTransact() const
 {
 	if (ODBC_TXN_CAPABILITY_UNKNOWN == _canTransact)

Data/SQLite/src/SessionImpl.cpp

@@ -52,7 +52,7 @@ const std::string SessionImpl::SQLITE_READ_COMMITTED  = "PRAGMA read_uncommitted
 SessionImpl::SessionImpl(const std::string& fileName, std::size_t loginTimeout):
 	Poco::Data::AbstractSessionImpl<SessionImpl>(fileName, loginTimeout),
 	_connector(Connector::KEY),
-	_pDB(0),
+	_pDB(nullptr),
 	_connected(false),
 	_isTransaction(false),
 	_transactionType(TransactionType::DEFERRED),
@@ -213,7 +213,7 @@ void SessionImpl::open(const std::string& connect)
 		while (true)
 		{
 			rc = sqlite3_open_v2(connectionString().c_str(), &_pDB,
-				SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_URI, NULL);
+				SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_URI, nullptr);
 			if (rc == SQLITE_OK) break;
 			if (!_pDB)
 				throw ConnectionFailedException(std::string(sqlite3_errstr(rc)));
@@ -240,7 +240,7 @@ void SessionImpl::close()
 	if (_pDB)
 	{
 		sqlite3_close_v2(_pDB);
-		_pDB = 0;
+		_pDB = nullptr;
 	}
 
 	_connected = false;

Data/include/Poco/Data/Session.h

@@ -170,12 +170,12 @@ public:
 
 	Session(const std::string& connector,
 		const std::string& connectionString,
-		std::size_t timeout = LOGIN_TIMEOUT_DEFAULT);
+		std::size_t loginTimeout = LOGIN_TIMEOUT_DEFAULT);
 		/// Creates a new session, using the given connector (which must have
 		/// been registered), and connectionString.
 
 	Session(const std::string& connection,
-		std::size_t timeout = LOGIN_TIMEOUT_DEFAULT);
+		std::size_t loginTimeout = LOGIN_TIMEOUT_DEFAULT);
 		/// Creates a new session, using the given connection (must be in
 		/// "connection:///connectionString" format).
 

Data/include/Poco/Data/SessionImpl.h

@@ -59,7 +59,7 @@ public:
 	static const int CURSOR_USE_NEVER = 2;
 
 	SessionImpl(const std::string& connectionString,
-		std::size_t timeout = LOGIN_TIMEOUT_DEFAULT);
+		std::size_t loginTimeout = LOGIN_TIMEOUT_DEFAULT);
 		/// Creates the SessionImpl.
 
 	virtual ~SessionImpl();

Data/src/PooledSessionImpl.cpp

@@ -149,7 +149,7 @@ void PooledSessionImpl::close()
 			}
 		}
 		_pHolder->owner().putBack(_pHolder);
-		_pHolder = 0;
+		_pHolder = nullptr;
 	}
 }
 

Data/src/SQLChannel.cpp

@@ -648,6 +648,8 @@ void SQLChannel::setProperty(const std::string& name, const std::string& value)
 		{
 			Path d(dir);
 			dir = d.makeDirectory().makeAbsolute().toString();
+			File f(dir);
+			if (!f.exists()) f.createDirectories();
 		}
 		_directory = dir;
 	}

Data/src/Session.cpp

@@ -33,9 +33,9 @@ Session::Session(Poco::AutoPtr<SessionImpl> pImpl):
 
 Session::Session(const std::string& connector,
 	const std::string& connectionString,
-	std::size_t timeout)
+	std::size_t loginTimeout)
 {
-	Session newSession(SessionFactory::instance().create(connector, connectionString, timeout));
+	Session newSession(SessionFactory::instance().create(connector, connectionString, loginTimeout));
 	swap(newSession);
 }
 

Data/src/SessionFactory.cpp

@@ -61,7 +61,7 @@ void SessionFactory::remove(const std::string& key)
 
 Session SessionFactory::create(const std::string& key,
 	const std::string& connectionString,
-	std::size_t timeout)
+	std::size_t loginTimeout)
 {
 	Poco::SharedPtr<Connector> ptrSI;
 	{
@@ -70,16 +70,16 @@ Session SessionFactory::create(const std::string& key,
 		if (_connectors.end() == it) throw Poco::NotFoundException(key);
 		ptrSI = it->second.ptrSI;
 	}
-	return Session(ptrSI->createSession(connectionString, timeout));
+	return Session(ptrSI->createSession(connectionString, loginTimeout));
 }
 
 
 Session SessionFactory::create(const std::string& uri,
-	std::size_t timeout)
+	std::size_t loginTimeout)
 {
 	URI u(uri);
 	poco_assert (!u.getPath().empty());
-	return create(u.getScheme(), u.getPath().substr(1), timeout);
+	return create(u.getScheme(), u.getPath().substr(1), loginTimeout);
 }
 
 

Data/src/SessionImpl.cpp

@@ -20,10 +20,10 @@ namespace Poco {
 namespace Data {
 
 
-SessionImpl::SessionImpl(const std::string& connectionString, std::size_t timeout):
+SessionImpl::SessionImpl(const std::string& connectionString, std::size_t loginTimeout):
 	_dbmsName("unknown"s),
 	_connectionString(connectionString),
-	_loginTimeout(timeout)
+	_loginTimeout(loginTimeout)
 {
 }