-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Opt-out from reusing SQL Server transport's connection #148
Merged
SimonCropp
merged 2 commits into
develop
from
add-option-to-not-use-transport-connection
Aug 25, 2017
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions
1
src/SqlPersistence.Tests/Integration/SqlTransportIntegrationTests.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Data.Common; | ||
using System.IO; | ||
using System.Linq; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
namespace NServiceBus | ||
{ | ||
using System; | ||
using System.Data.Common; | ||
using System.Threading.Tasks; | ||
using Extensibility; | ||
using Persistence; | ||
using Transport; | ||
|
||
public partial class SqlDialect | ||
{ | ||
internal abstract Task<CompletableSynchronizedStorageSession> TryAdaptTransportConnection(TransportTransaction transportTransaction, ContextBag context, Func<DbConnection> connectionBuilder, Func<DbConnection, DbTransaction, bool, StorageSession> storageSessionFactory); | ||
} | ||
} |
54 changes: 54 additions & 0 deletions
54
src/SqlPersistence/SynchronizedStorage/SqlDialect_MsSqlServer.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
namespace NServiceBus | ||
{ | ||
using System; | ||
using System.Data.Common; | ||
using System.Threading.Tasks; | ||
using System.Transactions; | ||
using Extensibility; | ||
using Persistence; | ||
using Transport; | ||
|
||
public partial class SqlDialect | ||
{ | ||
public partial class MsSqlServer | ||
{ | ||
internal override async Task<CompletableSynchronizedStorageSession> TryAdaptTransportConnection(TransportTransaction transportTransaction, ContextBag context, Func<DbConnection> connectionBuilder, Func<DbConnection, DbTransaction, bool, StorageSession> storageSessionFactory) | ||
{ | ||
if (DoNotUseTransportConnection) | ||
{ | ||
return null; | ||
} | ||
|
||
//SQL server transport in native TX mode | ||
if (transportTransaction.TryGet("System.Data.SqlClient.SqlConnection", out DbConnection existingSqlConnection) && | ||
transportTransaction.TryGet("System.Data.SqlClient.SqlTransaction", out DbTransaction existingSqlTransaction)) | ||
{ | ||
return storageSessionFactory(existingSqlConnection, existingSqlTransaction, false); | ||
} | ||
|
||
// Transport supports DTC and uses TxScope owned by the transport | ||
var scopeTx = Transaction.Current; | ||
if (transportTransaction.TryGet(out Transaction transportTx) && | ||
scopeTx != null && | ||
transportTx != scopeTx) | ||
{ | ||
throw new Exception("A TransactionScope has been opened in the current context overriding the one created by the transport. " | ||
+ "This setup can result in inconsistent data because operations done via connections enlisted in the context scope won't be committed " | ||
+ "atomically with the receive transaction. To manually control the TransactionScope in the pipeline switch the transport transaction mode " | ||
+ $"to values lower than '{nameof(TransportTransactionMode.TransactionScope)}'."); | ||
} | ||
var ambientTransaction = transportTx ?? scopeTx; | ||
if (ambientTransaction == null) | ||
{ | ||
//Other modes handled by creating a new session. | ||
return null; | ||
} | ||
var connection = await connectionBuilder.OpenConnection().ConfigureAwait(false); | ||
connection.EnlistTransaction(ambientTransaction); | ||
return storageSessionFactory(connection, null, true); | ||
} | ||
|
||
internal bool DoNotUseTransportConnection { get; set; } | ||
} | ||
} | ||
} |
23 changes: 23 additions & 0 deletions
23
src/SqlPersistence/SynchronizedStorage/SqlDialect_MySql.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
namespace NServiceBus | ||
{ | ||
using System; | ||
using System.Data.Common; | ||
using System.Threading.Tasks; | ||
using Extensibility; | ||
using Persistence; | ||
using Transport; | ||
|
||
public partial class SqlDialect | ||
{ | ||
public partial class MySql | ||
{ | ||
static Task<CompletableSynchronizedStorageSession> result = Task.FromResult((CompletableSynchronizedStorageSession)null); | ||
|
||
internal override Task<CompletableSynchronizedStorageSession> TryAdaptTransportConnection(TransportTransaction transportTransaction, ContextBag context, Func<DbConnection> connectionBuilder, Func<DbConnection, DbTransaction, bool, StorageSession> storageSessionFactory) | ||
{ | ||
// MySQL does not support DTC so we should not enlist if transport has such a transaction. | ||
return result; | ||
} | ||
} | ||
} | ||
} |
40 changes: 40 additions & 0 deletions
40
src/SqlPersistence/SynchronizedStorage/SqlDialect_Oracle.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
namespace NServiceBus | ||
{ | ||
using System; | ||
using System.Data.Common; | ||
using System.Threading.Tasks; | ||
using System.Transactions; | ||
using Extensibility; | ||
using Persistence; | ||
using Transport; | ||
|
||
public partial class SqlDialect | ||
{ | ||
public partial class Oracle | ||
{ | ||
internal override async Task<CompletableSynchronizedStorageSession> TryAdaptTransportConnection(TransportTransaction transportTransaction, ContextBag context, Func<DbConnection> connectionBuilder, Func<DbConnection, DbTransaction, bool, StorageSession> storageSessionFactory) | ||
{ | ||
// Oracle supports DTC so we should enlist in the transport's TransactionScope if present | ||
var scopeTx = Transaction.Current; | ||
if (transportTransaction.TryGet(out Transaction transportTx) && | ||
scopeTx != null && | ||
transportTx != scopeTx) | ||
{ | ||
throw new Exception("A TransactionScope has been opened in the current context overriding the one created by the transport. " | ||
+ "This setup can result in inconsistent data because operations done via connections enlisted in the context scope won't be committed " | ||
+ "atomically with the receive transaction. To manually control the TransactionScope in the pipeline switch the transport transaction mode " | ||
+ $"to values lower than '{nameof(TransportTransactionMode.TransactionScope)}'."); | ||
} | ||
var ambientTransaction = transportTx ?? scopeTx; | ||
if (ambientTransaction == null) | ||
{ | ||
//Other modes handled by creating a new session. | ||
return null; | ||
} | ||
var connection = await connectionBuilder.OpenConnection().ConfigureAwait(false); | ||
connection.EnlistTransaction(ambientTransaction); | ||
return storageSessionFactory(connection, null, true); | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking how weird this is going to look to someone who isn't also using the SQL Transport. The comment here needs to tell the user why this would be necessary, and the answer is only if you were using a different connection string between SQL Transport and SQL Persistence, right?
So that got me thinking - can't we just look at the connection strings and enlist only if they match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavidBoike that used to be an option with NHibernate but since SQL persistence uses
Func<DbConnection>
it would mean that for each message we need to create the persistence connection using the factory and check if connection string's match.Alternatively we can assume that the connection factory always returns connection with the same connection string and check only once but I am not sure if we can make such an assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we also can't detect when they have it misconfigured and throw an appropriate exception, because we don't even try creating a connection until we've determined we can't enlist, right?
I'm not sure I like that very much. I wouldn't want to wait around for an "invalid object name" exception with no further clue to what's really going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about trying to create the connection on strartup to capture the connection string there? We do that in SQL transport if user provides a
func
:https://github.com/Particular/NServiceBus.SqlServer/blob/develop/src/NServiceBus.SqlServer/SqlServerTransport.cs#L64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SzymonPobiega I like the connection string capture on startup idea.