Skip to content

fix(spanner-ado-net): prevent sync calls on async execution paths for DML stats#776

Open
olavloite wants to merge 1 commit into
mainfrom
ado-net-async-stats
Open

fix(spanner-ado-net): prevent sync calls on async execution paths for DML stats#776
olavloite wants to merge 1 commit into
mainfrom
ado-net-async-stats

Conversation

@olavloite

Copy link
Copy Markdown
Collaborator

Introduced asynchronous StatsAsync methods in ISpannerLib and GrpcLibSpanner, along with async GetStatsAsync and GetUpdateCountAsync helpers in Rows.cs.

This enables GetTotalUpdateCountAsync to run completely asynchronously. In StreamingRows, GetStatsAsync returns the local cached stats in memory, ensuring no remote blocking IPC calls are executed during asynchronous DML execution.

@olavloite olavloite requested review from a team as code owners June 24, 2026 12:44

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces asynchronous support for retrieving Spanner query and DML execution statistics, adding StatsAsync to the ISpannerLib interface and implementing GetStatsAsync and GetUpdateCountAsync in the Rows class. The review feedback highlights several critical issues: GetTotalUpdateCountAsync can miss update counts for DML with RETURNING clauses if rows are not consumed first; adding StatsAsync to ISpannerLib without a default implementation breaks backward compatibility; and redundant remote calls may occur when stats are null, which can be resolved by introducing and properly resetting a _statsLoaded boolean flag.

Comment thread spanner-ado-net/spanner-ado-net/SpannerLib/Rows.cs
Comment thread spanner-ado-net/spanner-ado-net/SpannerLib/ISpannerLib.cs Outdated
Comment thread spanner-ado-net/spanner-ado-net/SpannerLib/Rows.cs
Comment thread spanner-ado-net/spanner-ado-net/SpannerLib/Rows.cs
@olavloite olavloite force-pushed the ado-net-async-stats branch from 84ec46c to 4234295 Compare June 24, 2026 13:14
@olavloite

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces asynchronous methods for retrieving result set statistics and update counts (such as GetStatsAsync and GetUpdateCountAsync) in the Spanner ADO.NET library, along with corresponding unit tests. Feedback highlights a potential sync-over-async leak in GetStatsAsync where _stats.IsValueCreated remains false after loading stats asynchronously, which could trigger redundant synchronous IPC calls during subsequent synchronous operations. Re-initializing _stats with a lazy wrapper of the loaded stats is recommended to resolve this.

Comment thread spanner-ado-net/spanner-ado-net/SpannerLib/Rows.cs
@olavloite olavloite force-pushed the ado-net-async-stats branch from 4234295 to b38e2c5 Compare June 24, 2026 13:26
@olavloite

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces asynchronous support for retrieving query statistics and update counts in the Spanner ADO.NET provider. It adds StatsAsync to ISpannerLib and implements GetStatsAsync and GetUpdateCountAsync in Rows to prevent synchronous blocking (sync-over-async) when retrieving DML update counts. Comprehensive unit tests are also added to verify these asynchronous paths and cache clearing behaviors. The reviewer suggested optimizing the GetStatsAsync implementation in Rows.cs by modifying the Next() method to check _statsLoaded instead of re-initializing the _stats Lazy instance, which avoids unnecessary allocations.

Comment thread spanner-ado-net/spanner-ado-net/SpannerLib/Rows.cs Outdated
… DML stats

Introduced asynchronous StatsAsync methods in ISpannerLib and GrpcLibSpanner,
along with async GetStatsAsync and GetUpdateCountAsync helpers in Rows.cs.

This enables GetTotalUpdateCountAsync to run completely asynchronously. In
StreamingRows, GetStatsAsync returns the local cached stats in memory, ensuring
no remote blocking IPC calls are executed during asynchronous DML execution.
@olavloite olavloite force-pushed the ado-net-async-stats branch from b38e2c5 to 4fc90c3 Compare June 25, 2026 06:06
@olavloite

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces asynchronous support for retrieving ResultSetStats from a Rows object, preventing synchronous-over-asynchronous blocking calls. Key changes include adding StatsAsync to the ISpannerLib interface and its gRPC implementation, introducing GetStatsAsync and GetUpdateCountAsync in Rows.cs to fetch and cache stats asynchronously, and updating GetTotalUpdateCountAsync to utilize these asynchronous paths. Additionally, new unit tests were added in CommandTests.cs and RowsTests.cs to validate the asynchronous behavior and cache-clearing logic. I have no feedback to provide as there are no review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant