Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Scope without prepared transactions #8

Merged
merged 4 commits into from

3 participants

@roji
Owner

Hey Francisco and Josh, sorry for disappearing, had some busy times at work.

Here's an attempt at a patch which would remove the dependency on prepared transactions when a connection is closed within a TransactionScope. I added a unit test which reproduced the problem on stock Postgres installations: SystemTransactionTests.SimpleTransactionScopeWithImplicitEnlist.

My general approach was to check whether a local (non-prepared) transaction is still active in the NpgsqlPromotableSinglePhaseNotification when Close() is called. If so, we enter the "postponingClose" state instead of actually closing. Then, when the local transaction is terminated (because of a commit, rollback or promote) the connection is notified and does the real close. There are a few more tweaks (Dispose and reopen after close) but it should be pretty straightforward.

Note that this takes care of the pattern:
using (var conn1 = new NpgsqlConnection(...) {
conn1.Open(); conn1.Close();
conn1.Open(); conn1.Close();
}

But does not take care of the pattern:
using (var conn1 = new NpgsqlConnection(connString1) { ... }
using (var conn2 = new NpgsqlConnection(connString1) { ... }
As Josh suggested, since I think that would require handling stuff at the pool. Maybe a second commit?

Anyway, let me know what you think...

Note that there are also two tiny build cleanup commits before the real one...

Shay Rojansky and others added some commits
Shay Rojansky Removed unneeded NUnit references in unit tests
NUnit.Core, NUnit.Core.Interfaces, NUnit.Util, was causing build build
errors
3d61e1a
Shay Rojansky Corrected unit test project references
NpgsqlTests2010 now references Npgsql2010 instead of Npgsql2008
918167c
@roji roji Conn can be closed in TransactionScope without prepared transaction
Previously closing a connection within a TransactionScope triggered an
automatic promotion to a prepared transaction. Since by default Postgres
disables these, this made the TransactionScope unusable for many cases.
This is reproduced in
SystemTransactionTests.SimpleTransactionScopeWithImplicitEnlist, which was
failing in stock Postgres installations.

Instead of promoting, we now postpone the close until the (local)
transaction is terminated, i.e. until the TransactionScope ends.
fccc7b3
@roji roji Removed connection leak detection and handling
By Francisco's request
cefdbaf
@franciscojunior franciscojunior merged commit 55d7f3f into npgsql:master
@franciscojunior

Thank you very much for your patches, Shay! Good work. I'll get your changes in our cvs repository too.

@roji
Owner

Great Francisco, it's a pleasure to help!

@franciscojunior

Finally could apply your patch to cvs. Thanks again for your help!

@roji
Owner

Thanks Francisco :)

FYI Some support for ambient transactions was missing in mono and my patch for that was accepted a while ago (mono/mono#484), so in the next npgsql it should all work nicely together...

@roji roji deleted the roji:scope_without_prepared_transactions branch
@omatrot

Hi all,
This fix solved a problem where, in a WCF service, a local transaction was treated as a distributed transaction.
Now, in this same WCF service, when the transaction comes from the client, so it is now distributed, and should be treated with a prepared transaction, it is treated local instead.
Could we work on this together ?

@roji
Owner

I can try to find some time and look at this. To help out, can you please:

  • Open a separate issue for this on github, and
  • If possible, try to submit a testcase reproducing this?

Are you on mono or MS.NET? Which version?

@omatrot

Hi, Sorry for the late reply.
I'm using MS .NET4.5.

For a WCF service sample, please go the follwing link (MSDN Magazine article about Transactions in WCF) : http://msdn.microsoft.com/en-us/magazine/cc163432.aspx, use the code found in figure 5, and add some Npgsql code in the service.

Create a simple console client, use 'add service reference' to connect to the service. You could now make a call to the service inside a Transaction Scope as shown in figure 8.

I'll create a Git issue ASAP.

@omatrot

Just so you know, I've managed to create a fix that is working fine in my context. I'll do more regression testing before suggesting it.
Basically:
NpgsqlConnection.cs :

  • Introduced a new method called DistributedTransactionPreparePhaseEnded to mirror PromotableLocalTransactionEnded for the specific case of a pending distributed transaction.
  • Modified Close() to check if a distributed transaction is pending in order to postpone the call to RealClose(). NpgsqlTransactionCallbacks.cs :
  • PrepareTransaction() is modified to call connection.DistributedTransactionPreparePhaseEnded() once the transaction is prepared on the database. NpgsqlPromotableSinglePhaseNotification.cs :
  • Added a boolean named InDistributedTransaction.
  • Modified Enlist() to set InDistributedTransaction to true if the call to EnlistPromotableSinglePhase return false. NpgsqlResourceManager.cs :
  • Modified Enlist(Transaction tx) to call EnlistDurable on the transaction to specify EnlistmentOptions.EnlistDuringPrepareRequired instead of EnlistmentOptions.None. This has the effect of calling IEnlistmentNotification.Prepare.
  • Also assigned a new Guid each time a resource manager is created because for now, it is not implemented in a Windows Service to act as a proxy, and could be created multiple times in a server environment.
@roji
Owner
@roji roji referenced this pull request from a commit in roji/Npgsql
@roji roji Work on ambient transaction and related refactoring
- Removed the previous "postponed close" state on Connection, which allowed for partial connection
  reuse within an ambient transaction (see npgsql#8 and
  discussion on the list).
- When a Connection is closed while a TransactionScope is still active, its connector is detached
  and returned to the pool, but specially marked as AvailableWithTransaction. Any subsequent
  Connection within the same transaction and connection string will now preferably be attached to
  that connector, preventing escalation to prepared transactions.
- When an ambient transaction completes, all the connectors involved it in are notified. If a
  connector is detached (i.e. its connection has already been closed), it is returned to the pool
  and marked as Available.
- A similar flow has been implemented for non-pooled connections/connectors.
- The above required a refactor of Connector, Connection and some associated classes. In particular,
  to allow connections to be smoothly detached and reattached to connectors, many Npgsql components
  now references the connector instead of the connection, and the connection has been made as bare
  a frontend as possible.
- SSL callbacks are now always registered just before Connector.Open(), and removed right
  afterwards.
e07abe3
@gencer gencer referenced this pull request from a commit in gencer/Npgsql2
fxjr Applied Shay Rojansky patch. [#1011305] Scope without prepared transa…
…ctions. Check npgsql#8 for more info.
ade62ad
@kenjiuno kenjiuno referenced this pull request from a commit in kenjiuno/Npgsql
@kenjiuno kenjiuno Step #8: Add the VSIXManifestSchema.xsd to allow VsixManifest validat…
…ion on the build server
4ced51d
@kenjiuno kenjiuno referenced this pull request from a commit in kenjiuno/Npgsql
@kenjiuno kenjiuno Step #8: Add the VSIXManifestSchema.xsd to allow VsixManifest validat…
…ion on the build server
8403574
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 15, 2012
  1. Removed unneeded NUnit references in unit tests

    Shay Rojansky authored
    NUnit.Core, NUnit.Core.Interfaces, NUnit.Util, was causing build build
    errors
  2. Corrected unit test project references

    Shay Rojansky authored
    NpgsqlTests2010 now references Npgsql2010 instead of Npgsql2008
  3. @roji

    Conn can be closed in TransactionScope without prepared transaction

    roji authored
    Previously closing a connection within a TransactionScope triggered an
    automatic promotion to a prepared transaction. Since by default Postgres
    disables these, this made the TransactionScope unusable for many cases.
    This is reproduced in
    SystemTransactionTests.SimpleTransactionScopeWithImplicitEnlist, which was
    failing in stock Postgres installations.
    
    Instead of promoting, we now postpone the close until the (local)
    transaction is terminated, i.e. until the TransactionScope ends.
Commits on Nov 16, 2012
  1. @roji

    Removed connection leak detection and handling

    roji authored
    By Francisco's request
This page is out of date. Refresh to see the latest.
View
92 src/Npgsql/NpgsqlConnection.cs
@@ -122,6 +122,9 @@ public sealed class NpgsqlConnection : DbConnection, ICloneable
// Used when we closed the connector due to an error, but are pretending it's open.
private bool _fakingOpen;
+ // Used when the connection is closed but an TransactionScope is still active
+ // (the actual close is postponed until the scope ends)
+ private bool _postponingClose;
// Strong-typed ConnectionString values
private NpgsqlConnectionStringBuilder settings;
@@ -523,6 +526,11 @@ public new NpgsqlTransaction BeginTransaction(IsolationLevel level)
/// </summary>
public override void Open()
{
+ // If we're postponing a close (see doc on this variable), the connection is already
+ // open and can be silently reused
+ if (_postponingClose)
+ return;
+
CheckConnectionClosed();
NpgsqlEventLog.LogMethodEnter(LogLevel.Debug, CLASSNAME, "Open");
@@ -598,28 +606,50 @@ internal void EmergencyClose()
public override void Close()
{
NpgsqlEventLog.LogMethodEnter(LogLevel.Debug, CLASSNAME, "Close");
-
- if (connector != null)
- {
- Promotable.Prepare();
- // clear the way for another promotable transaction
- promotable = null;
- connector.Notification -= NotificationDelegate;
- connector.Notice -= NoticeDelegate;
+ if (connector == null)
+ return;
- if (SyncNotification)
- {
- connector.RemoveNotificationThread();
- }
+ if (promotable != null && promotable.InLocalTransaction)
+ {
+ _postponingClose = true;
+ return;
+ }
- NpgsqlConnectorPool.ConnectorPoolMgr.ReleaseConnector(this, connector);
+ ReallyClose();
+ }
+ private void ReallyClose()
+ {
+ NpgsqlEventLog.LogMethodEnter(LogLevel.Debug, CLASSNAME, "ReallyClose");
+ _postponingClose = false;
- connector = null;
-
- }
- }
+ // clear the way for another promotable transaction
+ promotable = null;
+
+ connector.Notification -= NotificationDelegate;
+ connector.Notice -= NoticeDelegate;
+
+ if (SyncNotification)
+ {
+ connector.RemoveNotificationThread();
+ }
+
+ NpgsqlConnectorPool.ConnectorPoolMgr.ReleaseConnector(this, connector);
+
+ connector = null;
+ }
+
+ /// <summary>
+ /// When a connection is closed within an enclosing TransactionScope and the transaction
+ /// hasn't been promoted, we defer the actual closing until the scope ends.
+ /// </summary>
+ internal void PromotableLocalTransactionEnded()
+ {
+ NpgsqlEventLog.LogMethodEnter(LogLevel.Debug, CLASSNAME, "PromotableLocalTransactionEnded");
+ if (_postponingClose)
+ ReallyClose();
+ }
/// <summary>
/// Creates and returns a <see cref="System.Data.Common.DbCommand">DbCommand</see>
@@ -653,25 +683,17 @@ public new NpgsqlCommand CreateCommand()
/// <b>false</b> when being called from the finalizer.</param>
protected override void Dispose(bool disposing)
{
- if (!disposed)
- {
- if (disposing)
- {
- NpgsqlEventLog.LogMethodEnter(LogLevel.Debug, CLASSNAME, "Dispose");
- Close();
- }
- else
- {
- if (FullState != ConnectionState.Closed)
- {
- NpgsqlEventLog.LogMsg(resman, "Log_ConnectionLeaking", LogLevel.Debug);
- NpgsqlConnectorPool.ConnectorPoolMgr.FixPoolCountBecauseOfConnectionDisposeFalse(this);
- }
- }
+ if (disposed)
+ return;
- base.Dispose(disposing);
- disposed = true;
+ if (disposing)
+ {
+ NpgsqlEventLog.LogMethodEnter(LogLevel.Debug, CLASSNAME, "Dispose");
+ Close();
}
+
+ base.Dispose(disposing);
+ disposed = true;
}
/// <summary>
@@ -900,7 +922,7 @@ private void CheckConnectionOpen()
_fakingOpen = false;
}
- if (connector == null)
+ if (_postponingClose || connector == null)
{
throw new InvalidOperationException(resman.GetString("Exception_ConnNotOpen"));
}
View
22 src/Npgsql/NpgsqlConnectorPool.cs
@@ -497,28 +497,6 @@ private static NpgsqlConnector CreateConnector(NpgsqlConnection Connection)
return new NpgsqlConnector(Connection.ConnectionStringValues.Clone(), Connection.Pooling, false);
}
-
- /// <summary>
- /// This method is only called when NpgsqlConnection.Dispose(false) is called which means a
- /// finalization. This also means, an NpgsqlConnection was leak. We clear pool count so that
- /// client doesn't end running out of connections from pool. When the connection is finalized, its underlying
- /// socket is closed.
- /// </summary>
- public void FixPoolCountBecauseOfConnectionDisposeFalse(NpgsqlConnection Connection)
- {
- ConnectorQueue Queue;
-
- // Prevent multithread access to connection pool count.
- lock (locker)
- {
- // Try to find a queue.
- if (PooledConnectors.TryGetValue(Connection.ConnectionString, out Queue) && Queue != null)
- {
- Queue.Busy.Remove(Connection.Connector);
- }
- }
- }
-
/// <summary>
/// Close the connector.
/// </summary>
View
5 src/Npgsql/NpgsqlPromotableSinglePhaseNotification.cs
@@ -36,6 +36,7 @@ internal class NpgsqlPromotableSinglePhaseNotification : IPromotableSinglePhaseN
private NpgsqlTransactionCallbacks _callbacks;
private INpgsqlResourceManager _rm;
private bool _inTransaction;
+ internal bool InLocalTransaction { get { return _npgsqlTx != null; } }
private static readonly String CLASSNAME = MethodBase.GetCurrentMethod().DeclaringType.Name;
@@ -89,6 +90,7 @@ public void Prepare()
_npgsqlTx.Cancel();
_npgsqlTx.Dispose();
_npgsqlTx = null;
+ _connection.PromotableLocalTransactionEnded();
}
}
}
@@ -114,6 +116,7 @@ public void Rollback(SinglePhaseEnlistment singlePhaseEnlistment)
_npgsqlTx.Dispose();
_npgsqlTx = null;
singlePhaseEnlistment.Aborted();
+ _connection.PromotableLocalTransactionEnded();
}
else if (_callbacks != null)
{
@@ -141,6 +144,7 @@ public void SinglePhaseCommit(SinglePhaseEnlistment singlePhaseEnlistment)
_npgsqlTx.Dispose();
_npgsqlTx = null;
singlePhaseEnlistment.Committed();
+ _connection.PromotableLocalTransactionEnded();
}
else if (_callbacks != null)
{
@@ -181,6 +185,7 @@ public byte[] Promote()
_npgsqlTx.Cancel();
_npgsqlTx.Dispose();
_npgsqlTx = null;
+ _connection.PromotableLocalTransactionEnded();
}
return token;
}
View
1  testsuite/noninteractive/NUnit20/BaseClassTests.cs
@@ -29,7 +29,6 @@
using NpgsqlTypes;
using NUnit.Framework;
-using NUnit.Core;
namespace NpgsqlTests
{
View
1  testsuite/noninteractive/NUnit20/CommandTests.cs
@@ -30,7 +30,6 @@
using Npgsql;
using NUnit.Framework;
-using NUnit.Core;
using System.Data;
using System.Globalization;
using System.Net;
View
1  testsuite/noninteractive/NUnit20/DataAdapterTests.cs
@@ -30,7 +30,6 @@
using NpgsqlTypes;
using NUnit.Framework;
-using NUnit.Core;
namespace NpgsqlTests
{
View
1  testsuite/noninteractive/NUnit20/DataReaderTests.cs
@@ -29,7 +29,6 @@
using NpgsqlTypes;
using NUnit.Framework;
-using NUnit.Core;
namespace NpgsqlTests
{
View
7 testsuite/noninteractive/NUnit20/NpgsqlTests2010.csproj
@@ -58,10 +58,7 @@
<WarningLevel>4</WarningLevel>
</PropertyGroup>
<ItemGroup>
- <Reference Include="nunit.core" />
- <Reference Include="nunit.core.interfaces" />
<Reference Include="nunit.framework" />
- <Reference Include="nunit.util" />
<Reference Include="System.configuration" />
<Reference Include="System.Core">
<RequiredTargetFramework>3.5</RequiredTargetFramework>
@@ -136,10 +133,10 @@
</BootstrapperPackage>
</ItemGroup>
<ItemGroup>
- <ProjectReference Include="..\..\..\src\Npgsql2008.csproj">
+ <ProjectReference Include="..\..\..\src\Npgsql2010.csproj">
<Project>{9D13B739-62B1-4190-B386-7A9547304EB3}</Project>
<Name>Npgsql2008</Name>
</ProjectReference>
</ItemGroup>
<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.Targets" />
-</Project>
+</Project>
View
33 testsuite/noninteractive/NUnit20/SystemTransactionsTest.cs
@@ -24,6 +24,39 @@ protected virtual string TheConnectionString
get { return _connString; }
}
+ [Test, Description("TransactionScope with a single connection, enlisting explicitly")]
+ public void SimpleTransactionScopeWithExplicitEnlist()
+ {
+ string connectionString = TheConnectionString;
+ using (var connection = new NpgsqlConnection(connectionString)) {
+ using (var scope = new TransactionScope()) {
+ connection.Open();
+ connection.EnlistTransaction(Transaction.Current);
+ var command = new NpgsqlCommand("insert into tablea(field_text) values (:p0)", connection);
+ command.Parameters.Add(new NpgsqlParameter("p0", "test"));
+ Assert.AreEqual(1, command.ExecuteNonQuery());
+ scope.Complete();
+ }
+ }
+ AssertNoTransactions();
+ }
+
+ [Test, Description("TransactionScope with a single connection, enlisting implicitly")]
+ public void SimpleTransactionScopeWithImplicitEnlist()
+ {
+ string connectionString = TheConnectionString + ";enlist=true";
+ using (var scope = new TransactionScope()) {
+ using (var connection = new NpgsqlConnection(connectionString)) {
+ connection.Open();
+ var command = new NpgsqlCommand("insert into tablea(field_text) values (:p0)", connection);
+ command.Parameters.Add(new NpgsqlParameter("p0", "test"));
+ Assert.AreEqual(1, command.ExecuteNonQuery());
+ }
+ scope.Complete();
+ }
+ AssertNoTransactions();
+ }
+
[Test]
public void DistributedTransactionRollback()
{
Something went wrong with that request. Please try again.