search

rebus-org / Rebus.Oracle

:bus: Oracle transport for Rebus
https://mookid.dk/category/rebus
Other
5 stars 10 forks source link

Add ambient transaction support #8

Closed thomasdc closed 5 years ago

thomasdc commented 5 years ago

Adds support for enlisting in an ambient transaction. Fixes issue #7.

Implementation is backwards compatible and similar to pull request 27 in Rebus.SqlServer.

mookid8000 commented 5 years ago

Looks great! 👍 one thing, though: Why did you change the async operations to be synchronous?

thomasdc commented 5 years ago

Because this line was the only async part + the async->sync change propagated to several other files.

I've replaced connection.OpenAsync() with connection.Open() because it's already done like that in Rebus.SqlServer (see here).

thomasdc commented 5 years ago

On another note, I'm wondering, do I need Rebus.TransactionScopes to EnlistRebus() in a transaction scope if I'm already using the transaction scope within Rebus.Oracle or Rebus.SqlServer via enlistInAmbientTransaction?

mookid8000 commented 5 years ago

Regarding async – I feel that it's a step back to remove async. IMO it would be better to take a step forward and start using the async APIs of the connection and command objects instead.

I don't have time to look this through right now, though – I'll get time to properly review the code this Friday.

thomasdc commented 5 years ago

In that case you will need to check with @larsw on why he added the following note in Rebus.SqlServer:

// do not use Async here! it would cause the tx scope to be disposed on another thread than the one that created it
connection.Open();
mookid8000 commented 5 years ago

Thanks for your contribution! 👍

It's out in Rebus.Oracle 2.0.0-a2 and Rebus.Oracle.Devart 2.0.0-a2