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

isValid has no impact on transaction state #218

Merged
merged 1 commit into from Dec 1, 2014

Conversation

Projects
None yet
4 participants
@alexismeneses
Contributor

alexismeneses commented Nov 22, 2014

Proposed fix for #214

As suggested by @ringerc, isValid() now sends an empty statement.

Empty statements are then detected at lower level to prevent a BEGIN to be sent to the backend.

ringerc added a commit that referenced this pull request Dec 1, 2014

Merge pull request #218 from alexismeneses/ISSUE_214
Connection.isValid() should have no impact on transaction state

See issue #214 for details.

@ringerc ringerc merged commit 5600ae0 into pgjdbc:master Dec 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@ringerc

This comment has been minimized.

Member

ringerc commented Dec 1, 2014

I haven't tested this in detail, but it looks sane at a read and it passes in CI. Thanks for following up the report with a patch.

@cowwoc

This comment has been minimized.

Contributor

cowwoc commented Dec 1, 2014

Looks good by visual inspection. But does this actually detect when a connection has gone down?

@davecramer

This comment has been minimized.

Member

davecramer commented Dec 1, 2014

Yes, why do you question it ?

@cowwoc

This comment has been minimized.

Contributor

cowwoc commented Dec 1, 2014

@davecramer Because I didn't see a new unit test that checks if it actually does. If you have pre-existing unit tests that already do so then feel free to ignore my comment :)

@alexismeneses

This comment has been minimized.

Contributor

alexismeneses commented Dec 1, 2014

@cowwoc IsValidTest.testIsValid() was already doing so

@cowwoc

This comment has been minimized.

Contributor

cowwoc commented Dec 1, 2014

@alexismeneses Okay, so I withdraw my comment. Thanks guys.

@ringerc

This comment has been minimized.

Member

ringerc commented Dec 1, 2014

IsValidTest.testIsValid()| was already doing so

Only for a normal close.

We probably should also test:

  • conn1: connect
  • conn1: select pg_backend_pid()
  • conn2: connect
  • conn2: pg_terminate_backend(pid)
  • conn1: isValid()

and a test where we unwrap the stream and close the underlying Socket or replace it with a simulated broken socket.

However, as always, time is a factor. So I think this is a "patches are welcome" situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment