-
Notifications
You must be signed in to change notification settings - Fork 5k
Conversation
@jimcarley, please let me know if you have any changes you'd like to see made before this is merged, or whether you're ok with this being merged as-is. Thanks! @ericstj, could you confirm (or deny :) I've done the packaging correctly? |
public System.Guid PromoterType { get; } | ||
public System.Transactions.TransactionInformation TransactionInformation { get { return default(System.Transactions.TransactionInformation); } } | ||
public event System.Transactions.TransactionCompletedEventHandler TransactionCompleted { add { } remove { } } | ||
//protected System.IAsyncResult BeginCommitInternal(System.AsyncCallback callback) { return default(System.IAsyncResult); } |
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.
protected System.IAsyncResult BeginCommitInternal [](start = 10, length = 49)
Is the standard for .NET Core to stick with Task or should we continue to use Begin/End?
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.
Sorry, I should have added a comment here.
Originally for .NET Core, we were exluciding the Begin/End methods in favor of including the Task methods. As part of bringing back lots of APIs, we're bringing back those Begin/End methods that were excluded. I did not remove any such APIs as part of the port.
However, that's not why these are commented out. These are actually part of the Mono APIs, even though they're not in the desktop implementation of System.Transactions. It's unclear to me why such "Internal" methods are exposed as protected in Mono, maybe it's a mistake, or maybe it's there for some corner-case scenario that was desired. In any event, I wasn't sure what to do with them contract-wise, so I left them here but commented out. I'll add a comment explaining.
|
Stephen, While I haven’t looked at all of the code, yet, I haven’t seen anything glaring that would prevent this pull request. I have seen at least one flaw in the ported Mono tests in IntResourceManager.cs and EnlistTest.cs. The return code from Transaction.EnlistPromotableSinglePhase is not really handled correctly. There is no check that EnlistPromotableSinglePhase returns “false” if there is already a PSPE or durable enlistment on the transaction. In the cases where 2 PSPE enlistments are attempted or a Durable followed by a PSPE, there is a check to ensure that there were no enlistment “failures”. This check is based on exceptions being thrown. But a “false” return value from Transaction.EnlistPromotableSinglePhase indicates that the PSPE enlistment could not be created. It isn’t really a “failure”, and thus no exception is thrown. The “false” is an indication that the PSPE enlistment was not possible, but a durable enlistment could be successful and that the caller should attempt such an enlistment. The above is not necessarily something that needs to be fixed right now. I just wanted to call it out. Jim From: Stephen Toub [mailto:notifications@github.com] @jimcarleyhttps://github.com/jimcarley, please let me know if you have any changes you'd like to see made before this is merged, or whether you're ok with this being merged as-is. Thanks! @ericstjhttps://github.com/ericstj, could you confirm I've done the packaging correctly? — |
|
||
if (Type == ResourceManagerType.Promotable) | ||
{ | ||
_transaction.EnlistPromotableSinglePhase(new PromotableSinglePhaseNotification(this)); |
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.
EnlistPromotableSinglePhase will not throw an exception if there is already another PSPE or a DurableEnlistment. Instead, it returns false. Right now the tests that exercise these scenarios do not take this into account. This scenario should be taken into account to correctly test EnlistPromotableSinglePhase. One option is to count a "false" return as an enlistment failure.
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.
Ok, so I should basically add an Assert.True around all calls to enlistPromotableSinglePhase that are supposed to succeed? Assuming yes, I'll do that, and if any then cause failures, I'll undo it and add a TODO for follow-up.
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.
No. There are a couple of tests that will induce this to return false, so an Assert.True will break those tests. I think a check for "false" is in order and the "failure" should be counted and verified by the two tests that do this.
In reply to: 71283510 [](ancestors = 71283510)
// that it will be properly released. | ||
internal sealed class SafeIUnknown : SafeHandle | ||
{ | ||
private SafeIUnknown() : base(IntPtr.Zero, true) { } |
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.
private [](start = 8, length = 7)
Why did you make this private? In the desktop code, this is internal, just like the other constructor.
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.
Initially I made it private because nothing was using this ctor to instantiate it; the parameterless ctor was only necessary for the P/Invokes that were creating it, and for that purpose private is fine.
But then I subsequently removed all of those P/Invokes, and now nothing is using this type at all, so I'll just remove the file entirely.
Will do. |
} | ||
} | ||
|
||
internal Transaction(DistributedTransaction oleTransaction) |
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.
oleTransaction [](start = 52, length = 14)
change the name of this parameter to something like "distributedTransaction"
|
This is the vast majority of the implementation, ported to corefx. The main impact is the reliance on OLE has been removed, as has the reliance on COM+. Distributed transactions aren't supported implicitly, but they can be supported via plugging in a non-MSDTC manager via the relevant interfaces.
This ports all of the Mono System.Transactions tests as well as some of the .NET Framework tests. There is plenty more cleanup that can be done; I primarily focused on porting them to xunit.
Thanks for all the feedback. I've addressed the issues and will merge once the CI build completes. |
And fix project.json versions
Add System.Transactions to corefx Commit migrated from dotnet/corefx@af0f5f1
This ports System.Transactions from desktop to corefx.
There is still more work to be done before this can be considered stable, e.g. adding more tests (in part to help verify behavior remains the same across the port), working through if/how to add built-in distributed transaction support, etc., but this is a good start and should be sufficient to primary use cases around TransactionScope.
cc: @danmosemsft, @jimcarley, @dmetzgar, @SriniBanala, @EricaMo, @DamianEdwards
Fixes https://github.com/dotnet/corefx/issues/2949