spec says setReadOnly will throw an exception if in a transaction or if connection closed

[davecramer]

https://github.com/awslabs/aws-advanced-jdbc-wrapper/blob/dc3f1e96666ad394635fa3e2dcb9e0ca21214128/wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java#L459

this method is called on a closed connection or this method is called during a transaction

[congoamz]

Hi @davecramer, thanks for pointing this out, I've created a ticket to resolve this

[congoamz]

Hi @davecramer, I've done some investigation and while the PG driver does follow the spec, MySQL allows users to call setReadOnly inside transactions without throwing an exception. With the current implementation, the plugin essentially just forwards the setReadOnly call to the underlying driver without doing anything special. So whether an exception is thrown or not depends on the underlying driver. I've chatted with the team and we are proposing to keep this implementation since throwing an exception here could break customer MySQL workflows, even if those workflows are a bit unusual. Let me know if you have any objections or if I can close this issue.

A small caveat: the plugin does throw an exception if setReadOnly(false) is called while connected to a reader and inside a transaction. This is because the user has indicated they want a read-write connection but we cannot switch to a writer, as we are already in a transaction. If setReadOnly(true) is called while in a transaction we just stick with the same connection and forward the call to the underlying driver.

[davecramer]

@congoamz ugh :( Fair enough. So the next question is what to do about a connection that is closed.
https://github.com/awslabs/aws-advanced-jdbc-wrapper/blob/dc3f1e96666ad394635fa3e2dcb9e0ca21214128/wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java#L465 has us doing something on a closed connection. This is also against contrary to the SPEC

[congoamz]

@davecramer As is the plugin will get a new working connection in these scenarios. I can change this to throw an exception instead though

[davecramer]

@congoamz I think it makes more sense to do this. Anyone calling methods on a closed connection has a broken app. I'm not sure we want to hide that from the developer.

[congoamz]

Just merged in a PR that throws an exception if setReadOnly is called on a closed connection, so I'll close this issue now