fix: synchronize access to the underlying transaction for ambient transactions #6616

olavloite · 2021-06-15T10:05:15Z

Synchronizes access to the underlying Spanner transaction to prevent multiple transactions from being created when parallel commands are executed on the ambient transaction.

See error and discussion in GoogleCloudPlatform/dotnet-docs-samples#1320 for background.

…nsactions Synchronizes access to the underlying Spanner transaction to prevent multiple transactions from being created if parallel commands are executed on the ambient transaction.

olavloite · 2021-06-15T10:05:43Z

cc @skuruppu

amanda-tarafa

Great catch! I just have a suggestion for using Lazy instead of a normal lock. I think we could avoid calling Task.Result etc. and in any case it would be cleaner I think?

jskeet · 2021-06-15T11:45:52Z

I'd definitely be happier using Lazy<T> if we can... but it's not really clear to me how it's expected to work, in that if one thread has started acquiring a transaction, we have to block if another thread asks for the transaction. I think I'll need to look a little closer at where VolatileResourceManager is used...

olavloite · 2021-06-15T12:50:36Z

Using Lazy would be an option, but as far as I understand, it is by design synchronous. That would mean that we would have to:

Change GetTransactionAsync (which is basically the initialization method) to be a synchronous method.
Add some custom code to make an AsyncLazy implementation.
Add a custom library that supports some kind of AsyncLazy.
Or enlighten me on what other options there might be ;-)

amanda-tarafa · 2021-06-15T13:27:48Z

You can have Lazy<Task<Transaction>> and then you would normally do await Lazy.Value. That would take care of not blocking on our Execute...Async methods. The methods that we are implementing from the ISinglePhaseNotification interface are still sync so we would have to block on those. I'm trying to confirm from MS docs that blocking there shouldn't be a problem since those methods shouldn't be called from multiple threads.

jskeet

I think I follow this and like it, but I'd like Amanda to check as well :)

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs

amanda-tarafa

LGTM with a couple of suggestions.

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs

amanda-tarafa · 2021-06-19T00:10:41Z

Added do not merge label just while we finish discussing internally.

…cess

olavloite · 2021-07-21T07:24:03Z

I removed the Do not merge label as we have decided to go ahead with these changes including the DML check.

olavloite · 2021-07-21T07:58:22Z

@amanda-tarafa @jskeet Do you have any last comments/questions before merging this?

jskeet · 2021-07-21T08:00:46Z

Will have a look in a bit - I have a couple of other things on this morning. (If Amanda is happy, she's welcome to merge it - she's typically a better reviewer than I am anyway!)

amanda-tarafa

LGTM with three small requests.

amanda-tarafa · 2021-07-21T08:26:32Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs

        public void Dispose()
        {
            try
            {
-                _transaction?.Dispose();
-                // Protect against multiple disposals
+                if (IsTransactionCreated)


Actually I think you want here _transaction is null only, without caring about the value having been created or not.

No, in this case we do care, because it could be that the VolatileResourceManager is being disposed without the SpannerTransaction ever having been initialized. _transaction is set to a non-null value in the constructor. The underlying SpannerTransaction is however only created if it is actually needed. So if Dispose() would be called before the transaction has been used, it would unnecessarily first create a new SpannerTransaction and then dispose it right afterwards.

Yep, you are right.

amanda-tarafa · 2021-07-21T08:36:04Z

...oogle.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.IntegrationTests/TransactionScopeTests.cs

+
+            Assert.True(reader.Read());
+            Assert.Equal(keys.Length, reader.GetInt32(0));
+            Assert.False(reader.Read());


nit: I would remove this one, your select is a COUNT and if that returns more than one row, that's a problem with the reader, not with this test.

amanda-tarafa · 2021-07-21T08:38:41Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+                scope.Complete();
+            }
+
+            spannerClientMock.Verify(client => client.CommitAsync(


The verification in the next line superseeds this one. I think this one can be removed.

I think it's better to have both, as they do check two different things:

The first verification checks that there is only one Commit call, regardless of the number of mutations in the request.

The second verification checks that the only Commit call actually contains 3 mutations.

Removing the first verification would mean that if two commit requests were sent from the client, one without any mutations and the other with 3 mutations, the test would still pass. That is one of the scenarios that we want to verify is not happening. (Checking the number of BeginTransaction calls is not a good measure, as multiple transactions could be prepared by the session pool.)

Ah yes, you are right.

amanda-tarafa

LGTM

amanda-tarafa · 2021-07-21T11:43:38Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerCommandTests.cs

+                scope.Complete();
+            }
+
+            spannerClientMock.Verify(client => client.CommitAsync(


Ah yes, you are right.

amanda-tarafa · 2021-07-21T11:44:14Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs

        public void Dispose()
        {
            try
            {
-                _transaction?.Dispose();
-                // Protect against multiple disposals
+                if (IsTransactionCreated)


Yep, you are right.

Changes in Google.Cloud.Spanner.Data version 3.12.0: - [Commit 7c6a6f1](googleapis@7c6a6f1): feat: add support for JSON data type in Spanner ([issue 6390](googleapis#6390)) - [Commit ac367e2](googleapis@ac367e2): feat: Regenerate all APIs to support self-signed JWTs - [Commit 61938b6](googleapis@61938b6): - feat(Spanner): Support comments and statement hints in untyped commands. - Alternative to [issue 6848](googleapis#6848) - Closes [issue 6847](googleapis#6847) - [Commit d26b04c](googleapis@d26b04c): fix: address review comments - [Commit d2025be](googleapis@d2025be): fix: use logger from SpannerSettings - [Commit b34f6f4](googleapis@b34f6f4): cleanup: fix comment + remove unnecessary import - [Commit fc7a41b](googleapis@fc7a41b): test: remove connectionstring tests and add settings test - [Commit 6016ef0](googleapis@6016ef0): - feat: support custom SpannerSettings in SessionPoolManager - Support setting custom SpannerSettings when creating a SessionPoolManager. - [Commit 0ab6b8b](googleapis@0ab6b8b): - feat: allow adding an additional version header - Allows adding an additional version header to the connection string. This will be - added to the `x-goog-api-client` header that is used by the underlying Spanner client. - Only a fixed set of values may be set for the header (currently only 'efcore' is - allowed), and the property is not intended for public use. - [Commit 250124f](googleapis@250124f): - fix: synchronize access to the underlying transaction for ambient transactions ([issue 6616](googleapis#6616)) - * fix: synchronize access to the underlying transaction for ambient transactions - Synchronizes access to the underlying Spanner transaction to prevent - multiple transactions from being created if parallel commands are - executed on the ambient transaction. - * test: add integration test - * fix: use Lazy for initialization - * chore: address review comments - * fix: remove unnecessary read call Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.12.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.12.0 - Release Google.Cloud.Spanner.Common.V1 version 3.12.0 - Release Google.Cloud.Spanner.Data version 3.12.0 - Release Google.Cloud.Spanner.V1 version 3.12.0

Changes in Google.Cloud.Spanner.Data version 3.12.0: - [Commit 7c6a6f1](7c6a6f1): feat: add support for JSON data type in Spanner ([issue 6390](#6390)) - [Commit ac367e2](ac367e2): feat: Regenerate all APIs to support self-signed JWTs - [Commit 61938b6](61938b6): - feat(Spanner): Support comments and statement hints in untyped commands. - Alternative to [issue 6848](#6848) - Closes [issue 6847](#6847) - [Commit d26b04c](d26b04c): fix: address review comments - [Commit d2025be](d2025be): fix: use logger from SpannerSettings - [Commit b34f6f4](b34f6f4): cleanup: fix comment + remove unnecessary import - [Commit fc7a41b](fc7a41b): test: remove connectionstring tests and add settings test - [Commit 6016ef0](6016ef0): - feat: support custom SpannerSettings in SessionPoolManager - Support setting custom SpannerSettings when creating a SessionPoolManager. - [Commit 0ab6b8b](0ab6b8b): - feat: allow adding an additional version header - Allows adding an additional version header to the connection string. This will be - added to the `x-goog-api-client` header that is used by the underlying Spanner client. - Only a fixed set of values may be set for the header (currently only 'efcore' is - allowed), and the property is not intended for public use. - [Commit 250124f](250124f): - fix: synchronize access to the underlying transaction for ambient transactions ([issue 6616](#6616)) - * fix: synchronize access to the underlying transaction for ambient transactions - Synchronizes access to the underlying Spanner transaction to prevent - multiple transactions from being created if parallel commands are - executed on the ambient transaction. - * test: add integration test - * fix: use Lazy for initialization - * chore: address review comments - * fix: remove unnecessary read call Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.12.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.12.0 - Release Google.Cloud.Spanner.Common.V1 version 3.12.0 - Release Google.Cloud.Spanner.Data version 3.12.0 - Release Google.Cloud.Spanner.V1 version 3.12.0

olavloite added 2 commits June 15, 2021 11:40

fix: synchronize access to the underlying transaction for ambient tra…

f64ea24

…nsactions Synchronizes access to the underlying Spanner transaction to prevent multiple transactions from being created if parallel commands are executed on the ambient transaction.

test: add integration test

6a75992

olavloite requested a review from a team as a code owner June 15, 2021 10:05

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 15, 2021

amanda-tarafa reviewed Jun 15, 2021

View reviewed changes

fix: use Lazy for initialization

447c798

jskeet reviewed Jun 16, 2021

View reviewed changes

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs Outdated Show resolved Hide resolved

amanda-tarafa approved these changes Jun 17, 2021

View reviewed changes

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs Outdated Show resolved Hide resolved

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/VolatileResourceManager.cs Outdated Show resolved Hide resolved

amanda-tarafa added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 19, 2021

olavloite added 3 commits June 27, 2021 12:41

Merge branch 'master' into spanner-synchronize-ambient-transaction-ac…

d02c0fb

…cess

chore: address review comments

a120785

Merge branch 'master' into spanner-synchronize-ambient-transaction-ac…

5a20380

…cess

olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 21, 2021

amanda-tarafa approved these changes Jul 21, 2021

View reviewed changes

fix: remove unnecessary read call

8b1f954

amanda-tarafa approved these changes Jul 21, 2021

View reviewed changes

olavloite merged commit 250124f into googleapis:master Jul 21, 2021

olavloite deleted the spanner-synchronize-ambient-transaction-access branch July 21, 2021 12:15

amanda-tarafa mentioned this pull request Aug 24, 2021

Release Spanner libraries version 3.12.0 #7032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: synchronize access to the underlying transaction for ambient transactions #6616

fix: synchronize access to the underlying transaction for ambient transactions #6616

olavloite commented Jun 15, 2021 •

edited

Loading

olavloite commented Jun 15, 2021

amanda-tarafa left a comment

jskeet commented Jun 15, 2021

olavloite commented Jun 15, 2021 •

edited

Loading

amanda-tarafa commented Jun 15, 2021

jskeet left a comment

amanda-tarafa left a comment

amanda-tarafa commented Jun 19, 2021

olavloite commented Jul 21, 2021

olavloite commented Jul 21, 2021

jskeet commented Jul 21, 2021

amanda-tarafa left a comment

amanda-tarafa Jul 21, 2021

olavloite Jul 21, 2021

amanda-tarafa Jul 21, 2021

amanda-tarafa Jul 21, 2021

olavloite Jul 21, 2021

amanda-tarafa Jul 21, 2021

olavloite Jul 21, 2021

amanda-tarafa Jul 21, 2021

amanda-tarafa left a comment

amanda-tarafa Jul 21, 2021

amanda-tarafa Jul 21, 2021

fix: synchronize access to the underlying transaction for ambient transactions #6616

fix: synchronize access to the underlying transaction for ambient transactions #6616

Conversation

olavloite commented Jun 15, 2021 • edited Loading

olavloite commented Jun 15, 2021

amanda-tarafa left a comment

Choose a reason for hiding this comment

jskeet commented Jun 15, 2021

olavloite commented Jun 15, 2021 • edited Loading

amanda-tarafa commented Jun 15, 2021

jskeet left a comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

amanda-tarafa commented Jun 19, 2021

olavloite commented Jul 21, 2021

olavloite commented Jul 21, 2021

jskeet commented Jul 21, 2021

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Jun 15, 2021 •

edited

Loading

olavloite commented Jun 15, 2021 •

edited

Loading