Functions - Java Spanner example uses DCL

[saturnism]

Describe the issue

GCF Java 11 Spanner sample uses Double checked locking. The pattern is known to be broken, should follow the lazy field init example

[olavloite]

@saturnism
I considered using the lazy field init idiom, but the problem is that it's use is discouraged if the initialization of the lazy field can fail. From the source that is referenced in the example itself:

While the implementation is an efficient thread-safe "singleton" cache without synchronization overhead, and better performing than uncontended synchronization,[4] the idiom can only be used when the construction of Something can be guaranteed to not fail. In most JVM implementations, if construction of Something fails, subsequent attempts to initialize it from the same class-loader will result in a NoClassDefFoundError failure.

In this case, the initialization of Spanner can fail, which is why I chose to implement the double checked locking idiom instead. To my knowledge, the double check locking idiom was only broken in Java 1.4 and earlier, and should be working correctly from Java 5 and up.

See for example https://www.oracle.com/technical-resources/articles/javase/bloch-effective-08-qa.html.

Or is there something else here that I'm missing?

[saturnism]

good call on the exception. it is quite a bit of code to show how to use spanner in a function.

does it matter whether it's checks for initialized or simply check for client != null ?

The current code feels like that it will:

1. first getClient call will throw the error
2. second time it gets called it will return null and cause npe?

Would it make sense for createSpanner method to be part of SpannerHolder?

Lastly, would, static initialization work in the class, or IODH? static init exceptions can also be caught if the init happens in a static block.

[skuruppu]

Friendly ping on this issue to see what the next steps are. @olavloite, do you have any thoughts on the last comment from @saturnism?

[olavloite]

Sorry, this one slipped of my radar.

does it matter whether it's checks for initialized or simply check for client != null ?

Yes, that does matter, as client could still be null after initialization (see below).

The current code feels like that it will:

1. first getClient call will throw the error
2. second time it gets called it will return null and cause npe?

No, if an error occurs during initialization, the initialized flag will be set to true and the error will be remembered. The same error that occurred during initialization will therefore also be thrown the next time getClient() is called.

My preferred solution would be if we could add something like the Apache commons LazyInitalizer in the core client libraries that we could use for this. I don't think Spanner is the only cloud client that might throw an exception during initialization, and that should therefore also be using a pattern like this. That would significantly reduce the amount of code in this example, but also make it easier for end users to initialize these kinds of clients.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[olavloite]

We have added a LazyInitializer to the Spanner client library, so this sample can be updated once version 3.0.1 of the Spanner client is included in the libraries-bom.