Synchronize access to @client_threads in LogStash::Outputs::Tcp #1270
Conversation
|
yes, makes sense. see inline comments. |
| @@ -75,6 +75,7 @@ def register | |||
| @logger.info("Starting tcp output listener", :address => "#{@host}:#{@port}") | |||
| @server_socket = TCPServer.new(@host, @port) | |||
| @client_threads = [] | |||
| @client_mutex = Mutex.new | |||
There was a problem hiding this comment.
I'd prefer @client_threads_lock to be consistent with the tcp input plugin which does something similar
| end | ||
| end | ||
| end | ||
| @client_threads_lock.synchronize do |
There was a problem hiding this comment.
to avoid any possible race condition, the whole Thread.start should be wrapped into a synchronize block. Without this, there is a possibility of the client thread finishing faster thus trying to @client_threads.delete(Thread.current) before client_thread was added into @client_threads.
|
LGTM Did you test it in your environment? adding specs for this would be really great! |
|
@colinsurprenant I don't really have the time right now to add a spec for this, but I have been using my original patch for a couple of months in a multi region environment (~20 servers) and it's working fine. |
|
Ok, I'll add an issue for this. Do you think you can test this patch revision in your environment too? |
|
@colinsurprenant sure I'll re-patch later today and let you know. |
|
@lmars great! Thanks! |
jordansissel
force-pushed
the
master
branch
from
f86287d to
a536eef
Compare
|
@lmars any update on the testing? |
|
@suyograo I have been using the latest patch for a few months without issue |
|
@lmars since we're moving the plugins to separate repositories, would you mind reopening this PR in https://github.com/logstash-plugins/logstash-output-tcp and rebasing against the current master? |
Logstash was consistently failing with the following error when a client connected to a TCP output:
Logstash at the time was receiving a heavy stream of events, so this is likely the result of a race condition accessing
@client_threadswhich is not thread safe, so this change synchronizes access to it across threads.