Release connection before completing the future in RequestHandler#onSet

Description

As part of the fix for JAVA-633, the release of the connection in RequestHandler#onSet was moved to a finally block (because we want to hold onto the connection in the case where we get an UNPREPARED response).

This was a bad idea because now the connection is not released until after the future has been set. If a client uses Future.transform without a separate executor, and their callback take a long time to execute (like executing another request synchronously), we end up holding on the connection way too long. See this discussion.

Refactor the code to release the connection first, like it was done before (will just need a bit of duplication to avoid doing it in the UNPREPARED case).

Environment

None

Pull Requests

None

Activity

Show:
Sylvain Lebresne
February 16, 2015, 8:26 AM

I'll note that changing the connection releasing is not a bad idea, the shorter we hold a connection the better, but as I commented in the linked discussion I don't really think it matters much for the case you describe in the sense that if you use a callback without a separate executor, your callback should not take a long time to execute (or you'll slow down the whole driver), and even less execute another request synchronously (or you'll deadlock).

Olivier Michallat
April 1, 2015, 12:58 PM

This affects the same code as JAVA-561, so I'll do it on the same branch to avoid extra merge work.

Andy Tolbert
April 22, 2015, 9:24 PM

I added ConnectionReleaseTest to validate that stream id is released before setting the future. Will mark this issue resolved once we've merged in the java561 branch.

Fixed

Assignee

Olivier Michallat

Reporter

Olivier Michallat

Labels

PM Priority

None

Reproduced in

None

Affects versions

Fix versions

Pull Request

None

Doc Impact

None

Size

None

External issue ID

None

External issue ID

None

Components

Priority

Minor
Configure