Mark both client and server as not worker safe #14
Conversation
andrewvc
force-pushed
the
declare_workers_not_supported
branch
from
d496e0a
to
63e9ae1
Compare
| @@ -38,6 +38,9 @@ class LogStash::Outputs::Tcp < LogStash::Outputs::Base | |||
| # event will be written as a single line. | |||
| config :message_format, :validate => :string, :deprecated => true | |||
|
|
|||
| # respond_to? check needed for backwards compatibility with < 2.2 Logstashes | |||
| declare_workers_not_supported! if self.respond_to?(:workers_not_supported!) | |||
There was a problem hiding this comment.
this should be :workers_not_supported without !, right?
~/projects/logstash (git)-[2.2] % grep workers_not_supported\! . -r
./docs/static/include/pluginbody.asciidoc: declare_workers_not_supported! if self.respond_to?(:workers_not_supported!)
./docs/static/include/pluginbody.asciidoc: # Does the same thing as declare_workers_not_supported!
./logstash-core/lib/logstash/outputs/base.rb: declare_workers_not_supported!
./logstash-core/lib/logstash/outputs/base.rb: def self.declare_workers_not_supported!(message=nil)
./logstash-core/lib/logstash/outputs/base.rb: self.class.declare_workers_not_supported!(message)
and in 2.1.1
/tmp/logstash-2.1.1 % grep workers_not_supported\! . -r
/tmp/logstash-2.1.1 %
There was a problem hiding this comment.
it seems pluginbody.asciidoc also needs to be fixed.
There was a problem hiding this comment.
@jsvd nope! workers_not_supported is the legacy instance method. That's why we call both in this plugin. In v3.0.0 we can remove the legacy calls.
There was a problem hiding this comment.
@jsvd I do see a bug here though, it should be declare_workers_not_supported! if self.respond_to?(:declare_workers_not_supported!). I need to fix that in the asciidocs as well.
|
Server is threaded per-connection and appears to be synchronous (doesn't use IO.select). The main thread uses Socket#accept, by my reading anyway. Client does use IO.select, but with the old pipeline (before your filter-output merge), output workers were clones of output instances, so this was safe. |
|
Oops, submitted before I was done. Continuing from above "so this was safe" - can you update the commit message to reflect the cause of the tcp output w/ client mode no longer being threadsafe? (cause being the pipeline change, or never was threadsafe, or whatever?) |
| @@ -1,5 +1,6 @@ | |||
| ## 2.0.3 | |||
| - Declare plugin as not worker safe for both server and client mode. (IO#select is not threadsafe) | |||
There was a problem hiding this comment.
I think server mode is worker safe. Can you add a comment in the code linking to where IO#select is noted as not threadsafe in JRuby (docs somewhere on jruby, if possible?)
There was a problem hiding this comment.
The server mode was already marked as workers_not_supported before my change. Is this not the case? It shouldn't be, AFAICT given that only one worker can bind to a port.
|
Note that this plugin hasn't been worker safe for the client mode before, but since the logstash-core patch that increased the number of output workers to match the filter workers, this limitation has become evident. logstash 2.1.1: |
|
@jsvd how odd! Thanks for showing me the exception. :) |
|
Same in 1.5.4: |
|
@jordansissel looking at the 2.1 codebase it looks like we were just creating new instances, not cloning see here. I think this has always been a bug. |
|
@jsvd has confirmed that 2.1 had this same issue. |
|
I cannot get 1.4.2 to do the same, so it seems the logic indeed changed from 1.4 to 1.5 |
|
I'm trying to understand why the client isn't threadsafe: so multiple pipeline workers (N threads) will reuse the same output plugin which has multiple workers (M) to send data out. So, potentially, all N threads can, at some point, use the same worker out of M and that is why more than 1 thread does a select on the same tcp socket? Shouldn't the access to the output worker pool be synchronized? |
|
@jsvd yes, but I assumed that with the JMM using the SizedQueue synchronizes all the variables in the thread. I need to reread https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5 but IIRC that is the case. I could be wrong however. Maybe we do need to have a per-output synchronized block. HOWEVER I doubt that because the problem goes away with one worker. The object is shared across threads in that case as well. |
This is due to IO#select not being threadsafe in JRuby Fixes logstash-plugins#13
andrewvc
force-pushed
the
declare_workers_not_supported
branch
from
315d561
to
6f27ea0
Compare
andrewvc
force-pushed
the
declare_workers_not_supported
branch
from
6f27ea0
to
5c3e73d
Compare
|
@jsvd @jordansissel can you guys help follow-up with me on this patch here? |
|
LGTM - +1 on marking this plugin (both server and client) as not worker/thread-safe until we can make it safe. |
|
this was addressed in #21 |
This is due to IO#select not being threadsafe in JRuby
Fixes #13