Spanner: Do not hand out a closed Spanner instance from SpannerOptions.getService()

[olavloite]

SpannerOptions caches any Spanner instance that has been created, and hands this cached instance out to all subsequent calls to SpannerOptions.getService(). This also included closed Spanner instances. The getService() method now returns an error if the Spanner instance has already been closed.

[codecov]

Codecov Report

Merging #5187 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5187   +/-   ##
=========================================
  Coverage      50.4%    50.4%           
  Complexity    23785    23785           
=========================================
  Files          2251     2251           
  Lines        226792   226792           
  Branches      24966    24966           
=========================================
  Hits         114320   114320           
  Misses       103865   103865           
  Partials       8607     8607

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52dc8d0...d2c1746. Read the comment docs.

[snehashah16]

I would err on side of caution when returning errors/exception. I would prefer we either evict entries from cache on closing spanner instances OR try getting another entry from the cache if at all it is closed.

[olavloite]

There's no real cache in this case. The default behavior is defined in the base class of SpannerOptions, com.google.cloud.ServiceOptions#getService():

  public ServiceT getService() {
    if (service == null) {
      service = serviceFactory.create((OptionsT) this);
    }
    return service;
  }

Overriding this method and returning a new instance instead of an existing (closed) instance, is possible.

[kolea2]

Can we explore something like what Sneha is suggesting and not throw an error back to the user?

[olavloite]

The SpannerOptions#getService() method is public and not final so we could easily implement alternatives. As far as I can see it, the following alternatives would be possible:

1. The current implementation in master: The method creates a new Spanner instance on the first call, and then returns this instance in every subsequent call. This is the default behavior that is defined in com.google.cloud.ServiceOptions, the base class of SpannerOptions. The drawback of this implementation is that it will also return a closed Spanner instance if the user has closed one that has been returned by the method.
2. The implementation in this PR: The method creates a new Spanner instance on the first call, and then returns this instance in every subsequent call, unless the Spanner instance has been closed. In that case, the method throws an exception. Pro: The user gets the exception at an earlier stage, instead of at the next database call, and the behavior of the method is consistent with the default for Service classes. Con: The user gets an exception.
3. Creating a new Spanner instance if the cached instance has been closed: The method creates a new Spanner instance on the first call, and then returns this instance in every subsequent call, unless the Spanner instance has been closed. In that case, the method creates a new instance and returns that. Pro: The method does not throw an exception and always returns a valid Spanner instance. Con: The method sometimes returns an existing instance and sometimes a new instance, depending on the state of a previously created instance.
4. Creating a new Spanner instance in all cases: The method creates a new Spanner instance on every call to the method. Pro: No shared Spanner instances. Con: Major change from the current behavior, possibly causing larger resource leaks. Anyone that currently calls SpannerOptions#getService() repeatedly without ever closing the returned Spanner instance, will run into out-of-memory conditions very quickly.

A quick scan of other Service classes show that these use the default implementation, i.e. option 1 above. My gut feeling is to stick to option 2 from above, as it has the least impact on the current behavior (the user now also gets an exception, just at a later moment), and keeps it in line with other Service classes. @kolea2 WDYT?

[kolea2]

I think option 3 is what I'm leaning toward, and I think it is closest to what @snehashah16 suggested (instead of throw an error, have the library manage and handle).

[snehashah16]

I vote for #3 as well. Just to be sure, it will return the same instance if the same Spanner options are passed in.

…

On Fri, May 17, 2019, 8:14 AM kolea2 ***@***.***> wrote: I think option 3 is what I'm leaning toward, and I think it is closest to what @snehashah16 <https://github.com/snehashah16> suggested (instead of throw an error, have the library manage and handle). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5187?email_source=notifications&email_token=AGX7VPFZIQXRYFN5XJD3ECLPV3DVJA5CNFSM4HNAHPM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVVBEUY#issuecomment-493490771>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGX7VPCBPHZZVLEA3E6H33LPV3DVJANCNFSM4HNAHPMQ> .

[olavloite]

In option 3:

SpannerOptions.getService() will return the same Spanner instance as long as the Spanner instance has not been closed. Once the instance has been closed, SpannerOptions.getService() will create a new instance and start returning that instance. This new instance will have the exact same properties and behave the same as the initial instance, but it will not be the same instance. It is not possible to reopen an already closed Spanner instance.

The potential downside that I see with this approach is that we 'encourage' opening and closing Spanner instances during the lifetime of an application, while users should normally keep a Spanner instance open during the entire lifetime of the application. Closing a Spanner instance means that its session pool will be closed and all sessions will be deleted. Opening a new Spanner instance means initializing a new session pool.

It's a small change, so I'll make a separate PR with option 3 so it can be reviewed.

[ajaaym]

I think the correct usage is SpannerOptions.getDefaultInstance().getService() or something like this SpannerOptions.getDefaultInstance().toBuilder().set*().build().getService() once the service returned by the SpannerOption is closed. ServiceOptions is not meant to be a factory for Service.

[sduskis]

Option #3 should cache the service and Rpc, until it's closed. After that, it should create a new Service/Rpc and cache it until it's closed (and repeat). That seems like a straightforward algorithm.

[olavloite]

@sduskis @ajaaym @kolea2
Just to be sure everyone is on the same page here:

• Option 3 is implemented in Spanner: Create new instance if existing Spanner is closed #5200 and does exactly what @sduskis mentioned in the above comment.
• Option 2 is implemented in this PR. It throws an exception at the moment that the user requests a Spanner instance from a SpannerOptions that has been closed.
• Option 1 is the current behavior in master. This is to a certain degree the behavior that @ajaaym mentions in comment Spanner: Create new instance if existing Spanner is closed #5200 (comment), except that it now does not give the user any hints on how to proceed. It just throws an exception when one of the methods on the Spanner instance is invoked.

[sduskis]

@olavloite, I don't see how the service is cached after the initial service is closed. Perhaps we can do a VC.

[kolea2]

Closing in favor of #5200