Add TLS/SSL channel close timeouts #37246
Merged
Conversation
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
Collaborator
|
Pinging @elastic/es-security |
Contributor
Author
|
run gradle build tests 2 |
Tim-Brooks
changed the title
s1monw
approved these changes
Jan 9, 2019
Contributor
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
I left some comments. looks good though. I was pretty confused about the use of ThreadLocal in your description. Maybe you can change that?
| } | ||
| } | ||
|
|
||
| public interface Cancellable { |
Contributor
There was a problem hiding this comment.
can we just use Runnable instead?
|
|
||
| private final PriorityQueue<DelayedTask> tasks = new PriorityQueue<>(Comparator.comparingLong(DelayedTask::getDeadline)); | ||
|
|
||
| public Cancellable schedule(Runnable task, TimeValue timeValue) { |
Contributor
There was a problem hiding this comment.
I think this is trappy, can we only have the scheduleAtRelativeTime method instead?
| return scheduleAtRelativeTime(task, System.nanoTime() + timeValue.nanos()); | ||
| } | ||
|
|
||
| public Cancellable scheduleAtRelativeTime(Runnable task, long relativeNanos) { |
| * A basic priority queue backed timer service. The service is thread local and should only be used by a | ||
| * single nio selector event loop thread. | ||
| */ | ||
| public class NioTimer { |
Contributor
There was a problem hiding this comment.
I think the Nio in the name is obsolete?
Contributor
Author
|
run gradle build tests 1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Closing a channel using TLS/SSL requires reading and writing a
CLOSE_NOTIFY message (for pre-1.3 TLS versions). Many implementations do
not actually send the CLOSE_NOTIFY message, which means we are depending
on the TCP close from the other side to ensure channels are closed. In
case there is an issue with this, we need a timeout. This commit adds a
timeout to the channel close process for TLS secured channels.
As part of this change, we need a timer service. We could use the
generic Elasticsearch timeout threadpool. However, it would be nice to
have a local to the nio event loop timer service dedicated to network needs. In
the future this service could support read timeouts, connect timeouts,
request timeouts, etc. This commit adds a basic priority queue backed
service. Since our timeout volume (channel closes) is very low, this
should be fine. However, this can be updated to something more efficient
in the future if needed (timer wheel). Everything being local to the event loop
thread makes the logic simple as no locking or synchronization is necessary.