Skip to content

chore: remove potential RPC calls in finalizer calls#771

Open
olavloite wants to merge 4 commits into
mainfrom
no-rpcs-in-finalizer-code
Open

chore: remove potential RPC calls in finalizer calls#771
olavloite wants to merge 4 commits into
mainfrom
no-rpcs-in-finalizer-code

Conversation

@olavloite

Copy link
Copy Markdown
Collaborator

Remove potential RPC calls when finalizer calls are executed.

Remove potential RPC calls when finalizer calls are executed.
@olavloite olavloite requested review from a team as code owners June 24, 2026 07:17

@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 updates the Dispose(bool disposing) methods across several classes (AbstractLibObject, GrpcLibSpanner, and Server) to ensure that managed resources are only cleaned up when disposing is true, preventing finalizers from throwing exceptions or executing network operations. It also adds unit tests to verify this behavior. The review feedback correctly identifies that the new test Finalizer_DoesNotThrow_IfConnectionIsClosed is a false positive because closing the connection beforehand sets the disposed flag, causing the finalizer call to return early without executing the target logic; updating the test to run on an open connection is recommended.

Comment thread spanner-ado-net/spanner-ado-net-tests/ConnectionTests.cs
@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 updates the Dispose(bool disposing) methods in AbstractLibObject, GrpcLibSpanner, and Server to ensure that resource cleanup is only performed when disposing is true, preventing issues during finalization. It also adds unit tests to verify this behavior. The feedback suggests wrapping the Stop() call in Server.Dispose in a try-catch block to prevent exceptions from propagating during disposal, and explicitly specifying parameter types when using reflection in TransactionTests to avoid potential ambiguity.

Comment thread spanner-ado-net/spanner-ado-net/SpannerLibGrpcServer/Server.cs
Comment thread spanner-ado-net/spanner-ado-net-tests/TransactionTests.cs Outdated
@olavloite olavloite force-pushed the no-rpcs-in-finalizer-code branch from 6591f31 to 4e972e6 Compare June 24, 2026 08:56
@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 refactors the disposal and finalization logic across several classes, including AbstractLibObject, GrpcLibSpanner, and Server, to ensure that cleanup operations are only executed when explicitly disposing (disposing is true) rather than during finalization. It removes unnecessary finalizers and adds corresponding unit tests to verify that finalizers do not trigger these cleanup or rollback actions. There are no review comments to address, and I have no additional feedback to provide.

@olavloite olavloite requested a review from sakthivelmanii June 26, 2026 09:16
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