Allow for timeout while acquiring lock in StrictConnPool.#163
Merged
ok2c merged 3 commits intoapache:masterfrom Dec 13, 2019
Merged
Allow for timeout while acquiring lock in StrictConnPool.#163ok2c merged 3 commits intoapache:masterfrom
ok2c merged 3 commits intoapache:masterfrom
Conversation
ok2c
requested changes
Dec 11, 2019
| if (request.isDone()) { | ||
| this.completedRequests.add(request); | ||
| acquiredLock = this.lock.tryLock(requestTimeout.getDuration(), requestTimeout.getTimeUnit()); | ||
| } catch (final InterruptedException ignored) { |
Member
There was a problem hiding this comment.
@cwildman InterruptedException handling logic does not look quite right to me. At the very least one must call Thread.currentThread().interrupt() when catching and not re-throwing InterruptedException. I also think we should either cancel request at that point or propagate the exception to the caller.
Contributor
Author
There was a problem hiding this comment.
Makes sense. I updated to set interrupted again and immediately return a failed future with the InterruptedException.
ok2c
reviewed
Dec 12, 2019
| acquiredLock = this.lock.tryLock(requestTimeout.getDuration(), requestTimeout.getTimeUnit()); | ||
| } catch (final InterruptedException interruptedException) { | ||
| Thread.currentThread().interrupt(); | ||
| future.failed(interruptedException); |
Member
There was a problem hiding this comment.
@cwildman One last minor thing. Would not future#cancel() be more appropriate here?
Contributor
Author
There was a problem hiding this comment.
Yeah that does seem right.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I mentioned in the mailing list that a connection request does not fail immediately when the time to acquire the StrictConnPool lock is longer than the requestTimeout. Instead it will only fail when the lock is finally acquired and the deadline is checked in processPendingRequest(). @ok2c agreed and suggested I propose a PR.
This PR changes StrictConnPool.lease to use the requestTimeout when attempting to grab the lock. If the lock can't be acquired within the timeout the request is failed with a DeadlineTimeoutException.
A few comments: