Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bigquery/storage/managedwriter): context refactoring #8275

Merged
merged 9 commits into from
Jul 21, 2023

Conversation

shollyman
Copy link
Contributor

@shollyman shollyman commented Jul 17, 2023

This PR refactors how contexts are propagated/retained in the managedwriter package. Prior to multiplexing, each writer had a dedicated connection, and context could be shared between writer and the connection. Multiplexing decoupled these, and this PR addresses some issues with those changes.

Most notably, the library (incorrectly) used the context provided when instantiating a writer via NewManagedStream to derive the context for the connection pool, which would cause issues if said writer was closed before other writers in the pool completed/closed.

Similarly, (re)-opening a connection would reuse the same context, and certain failure scenarios (namely, server-close on an idle connection) could cause slow leak of grpc resources due to fully signalling disposal of the connection from the client side.

This CL corrects these issues by refactoring some of the key context lifecycles.

First, we use the context provided when instantiating the client (e.g. NewClient) as the root context when constructing connection pool and connections. It also preserves the desired property of making shutdown straightforward.

In the second, two key changes of note:

  • This CL changes the internal signature for our connection opening logic to accept a context as part of the signature, as opposed to binding context once when define our opening closure. Now, opening uses the connection's current context.
  • This CL now also cancels the previous context and recreates a new retained context and cancellation from its parent pool when we trigger reconnection logic. This also slightly alters the signature for our receiver goroutine to accept its context rather than referencing it from the connection itself, since the context lifecycles now differ.

Finally, this CL adds the requisite additional context expiry checks as we have potentially three contexts of note: the connection, the writer instance, and the request context (for a specific AppendRows request).

Fixes: #8232
Fixes: #8272

This PR refactors how contexts are retained in the managedwriter
package.  Prior to multiplexing, each writer had a dedicated connection,
and context could be shared between writer and the connection.

Now, the relationship between writers and their backing connections is
more nuanced.  Now, we use the context provided as part of client
instantiation (e.g. NewClient) for connection and pool management, and
context provided in NewManagedWriter is scoped to _just_ that specific
writer.

This corrects the previous problem where the context for the first
writer in a connection pool would be used for pool and connection
management, and thus a cancellation of a single  writer's context
could trigger writes from other writer instances to fail.
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigquery Issues related to the BigQuery API. labels Jul 17, 2023
@shollyman shollyman marked this pull request as ready for review July 20, 2023 16:43
@shollyman shollyman requested review from a team as code owners July 20, 2023 16:43
@shollyman shollyman requested a review from Neenu1995 July 20, 2023 16:43
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Jul 20, 2023
@shollyman shollyman merged commit c4104ea into googleapis:main Jul 21, 2023
@shollyman shollyman deleted the context-propagation branch July 21, 2023 16:58
gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 24, 2023
shollyman added a commit to shollyman/google-cloud-go that referenced this pull request Aug 29, 2023
The context refactoring in googleapis#8275 introduced two possible sources of
deadlocks in the ManagedStream AppendRows call path.  This PR addresses
those, and augments deadlock testing to cover this case.

Fixes: googleapis#8505
gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 29, 2023
The context refactoring in #8275 introduced two possible sources of deadlocks in the ManagedStream AppendRows call path.  This PR addresses those, and augments deadlock testing to cover this case.

Fixes: https://github.com/googleapis/google-cloud-go/issues/8505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. size: l Pull request size is large.
Projects
None yet
2 participants