Fix socksify incompatibility
#999
Conversation
While `dalli` doesn't depend on `resolv-replace`, we want to ensure it still works if it is required by the host application. This regression test ensures we don't break compatibility, and adds a framework allowing us to test compatibility with other gems in the future.
There was a problem hiding this comment.
The first commit is from #998, which I made as a separate PR because I think it makes sense to have the compatibility tests even if we decide to simply go back to exclusively relying on Timeout instead of connect_timeout:, as was discussed on #989 (comment).
| @@ -97,15 +97,14 @@ def self.open(host, port, options = {}) | |||
| end | |||
| end | |||
|
|
|||
| TCPSOCKET_ORIGINAL_INITIALIZE_PARAMETERS = [[:rest].freeze].freeze | |||
There was a problem hiding this comment.
This is memoized so we don't create duplicate arrays every time.
| if RUBY_VERSION >= '3.0' && | ||
| !::TCPSocket.private_instance_methods.include?(:original_resolv_initialize) | ||
| ::TCPSocket.instance_method(:initialize).parameters == TCPSOCKET_ORIGINAL_INITIALIZE_PARAMETERS |
There was a problem hiding this comment.
This just checks for the original parameters ([[:rest]]), but we could also use connect_timeout if we detect any of
parameters.include?([:key, :connect_timeout])parameters.include?([:keyreq, :connect_timeout])parameters.any? { |type, _name| type == :keyrest }
I kept it simple, as these would increase the runtime penalty, and I'm not aware of any gems that do monkey patch TCPSocket#initialize correctly, nor am I certain we must use connect_timeout: instead of Timeout in those cases. 🤷
|
I see
in the output of the only test runner that failed for the first commit, and similarly
in the output of the only test runner that failed for the second commit. I feel like I've got flakey tests due to a bunch of the tests and helpers using hard coded ports instead of grabbing a free or simply random port. I've opened #1000, which switches all test ports to be dynamically picked. |
Similarly to `resolv-replace`, `sockify` monkey patches `TCPSocket#initialize` which causes an error if it is called with `connect_timeout:`. Rather than check only for `resolv-replace`'s specific method, we can check if if the method's parameters have changed.
sambostock
force-pushed
the
socksify-compatibility
branch
from
0530aea
to
916d0fd
Compare
Similarly to
resolv-replace,sockifymonkey patchesTCPSocket#initializewhich causes an error if it is called withconnect_timeout:.Rather than check only for
resolv-replace's specific method, we can check if if the method's parameters have changed.This PR builds on #998 and adds the
socksifygem to the list of gems to smoke test compatibility with.Closes #996
resolv-replacecompatibility test #998 is merged