Fixed SSL handshake hang indefinitely with proxy setup

[kaisecheng]

Fix: #1033

The socket timeout of Apache HTTP client is unset in handshake phase (upstream issue)
This commit set the default socket config to PoolingHttpClientConnectionManager explicitly, so the timeout is non zero

[kaisecheng]

set the socket config to connection manager explicitly

[yaauie]

I am wary of a patch that replicates so much of manticore's internals, and would prefer we scope this in a way that doesn't mess with the client's ivars or attempt to replicate more observed behaviour than necessary.

I may be reading it wrong, but my current reading of the code is that we should be able to hook the simpler zero-side-effect pool_builder method, invoke its super method, and tap the result applying only our changes. That way if manticore changes the internals of pool to fix other issues, our patch doesn't silently stomp on upstream fixes.

Maybe something like:

module PoolBuilderSocketTimeoutPatch
  def pool_builder(options)
    super(options).tap do |cm|
      socket_config_builder = SocketConfig.custom
      socket_config_builder.set_so_timeout(options.fetch(:socket_timeout, Manticore::Client::DEFAULT_SOCKET_TIMEOUT) * 1000)
      socket_config_builder.set_tcp_no_delay(options.fetch(:tcp_no_delay, true))
      cm.set_default_socket_config socket_config_builder.build
    end
  end
end

Manticore::Client.instance_exec { prepend PoolBuilderSocketTimeoutPatch }

[yaauie]

👍 to getting an upstream fix merged already 🎉 .

I've added some commentary here, which may be obviated if we can get @cheald to put together a manticore release with this and other fixes 🤞🏼 .

[yaauie]

I am wary of a patch that replicates so much of manticore's internals, and would prefer we scope this in a way that doesn't mess with the client's ivars or attempt to replicate more observed behaviour than necessary.

I may be reading it wrong, but my current reading of the code is that we should be able to hook the simpler zero-side-effect pool_builder method, invoke its super method, and tap the result applying only our changes. That way if manticore changes the internals of pool to fix other issues, our patch doesn't silently stomp on upstream fixes.

Maybe something like:

module PoolBuilderSocketTimeoutPatch
  def pool_builder(options)
    super(options).tap do |cm|
      socket_config_builder = SocketConfig.custom
      socket_config_builder.set_so_timeout(options.fetch(:socket_timeout, Manticore::Client::DEFAULT_SOCKET_TIMEOUT) * 1000)
      socket_config_builder.set_tcp_no_delay(options.fetch(:tcp_no_delay, true))
      cm.set_default_socket_config socket_config_builder.build
    end
  end
end

Manticore::Client.instance_exec { prepend PoolBuilderSocketTimeoutPatch }

[yaauie]

While I see the value in the desire to do this, it reaches into internals that we do not own, so we can't rely on it working in future releases.

[yaauie]

If possible, I would prefer we create a module to contain our patch, and to mix that module into Manticore::Client (instead of monkey-patching Manticore::Client directly). I would also prefer for that module to be scoped within the plugin, so if we have to chase it down from some other plugin's use of manticore, we can see the patch on the client's ancestor chain.

[yaauie]

Generally I consider a require_relative with more than one ../ in it to be a warning sign that the file we are requiring probably isn't as well-scoped as it should be.

[cheald]

to getting an upstream fix merged already .

I've added some commentary here, which may be obviated if we can get @cheald to put together a manticore release with this and other fixes 🤞🏼 .

I'll get a new release pushed out!

[cheald]

https://rubygems.org/gems/manticore/versions/0.7.1-java - v0.7.1 released.

[yaauie]

While either approach is technically messing with Elasticsearch's private internals, I greatly prefer mocking methods over directly manipulating the ivar state of an object we don't own.

Both are liable to create problems down the track if the Elasticsearch gem decides to change their internals, but with a method mock we have both a greater sense of the intent behind the patch and a somewhat smaller likelihood of creating problems (e.g., if the ivar takes on a different semantic meaning we risk stomping on it with something invalid; while if the method signature changes we risk not intercepting).

Suggested change

	client = Elasticsearch::Client.new(:hosts => [get_host_port])
	client.instance_variable_set("@verified", true) # bypass client side version checking
	client
	Elasticsearch::Client.new(:hosts => [get_host_port]).tap do |client|
	allow(client).to receive(:verify_elasticsearch).and_return(true)
	end

[kaisecheng]

Thanks for the suggestions. The client team plans to release a version that relaxes the checking in 7.16. This hack is for a short-term transition. But I think it is no harm to take your suggestion too

[yaauie]

LGTM