Skip to content

Conversation

@jeremyrsmith
Copy link
Collaborator

This fixes an issue wherein the service isn't closed when the parse
phase of a prepared statement fails. This causes that connection to
hang. Instead, the parse failure should cause the service to be closed
(just as the successful completion of the query closes the service, and
the failure at any other point during the process does the same).

This fixes an issue wherein the service isn't closed when the parse
phase of a prepared statement fails.  This causes that connection to
hang.  Instead, the parse failure should cause the service to be closed
(just as the successful completion of the query closes the service, and
the failure at any other point during the process does the same).
@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage remained the same at 16.348% when pulling 1d45eda on jeremyrsmith:fix-parse-error-issue into ca5eb03 on finagle:master.

parse(sql, Some(service)).map { name =>
new PreparedStatementImpl(name, service)
}.onFailure {
err => service.close()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't' we just close it anyway (no matter if it's a failure or success)?

.ensure(service.close())

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't close it at this point if it's a success, because the service still has more phases to execute after Parse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PreparedStatementImpl needs to use the same service that Parse was executed on (because that session is where the prepared statement lives)

Sync needs to be issued on the connection before the service is closed.  Also, prepareAndExecute needs the same treatment.
@coveralls
Copy link

coveralls commented Jul 18, 2016

Coverage Status

Coverage remained the same at 16.058% when pulling b4932da on jeremyrsmith:fix-parse-error-issue into ca5eb03 on finagle:master.

@jeremyrsmith jeremyrsmith merged commit 0200ba2 into finagle:master Sep 2, 2016
@jeremyrsmith jeremyrsmith deleted the fix-parse-error-issue branch September 2, 2016 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants