Fixes createSocket being called with null proxy when proxy is null

[Sineaggi]

This is a follow-up to this fix 6d60624 where the null is handled in the implementation.

IMHO the null-check should be pulled out higher, when createSocket is called. As it stands, there's no way for createSocket(HttpContext to be called anymore. Any implementation of ConnectionSocketFactory by way of extending/inheriting existing implementations (SSL or Plain) would get caught by having to override both createSocket(HttpContext) and createSocket(Proxy, HttpContext). This is the case for docker-java, and is causing this issue docker-java/docker-java#2290

[ok2c]

@Sineaggi This looks wrong. The check for the proxy being non null should be performed by the method.

[twarner-sofi]

Then perhaps this code should be

return proxy != null ? new Socket(proxy) : createSocket(context);

As it stands, any overridden implementation of createSocket(HttpContext) is ignored, which is a breaking change.

[ok2c]

@twarner-sofi No, it is not. It is used by the default method of the interface. There is no breaking change

    @Internal
    default Socket createSocket(Proxy proxy, HttpContext context) throws IOException {
        return createSocket(context);
    }

[twarner-sofi]

In the interface, yes, but that delegation is not reflected in PlainConnectionSocketFactory's implementation of the method. This class is not final and thus users can extend it, overriding createSocket. If that is incorrect usage and this class was meant to be final, I think that should at least be documented in the class's javadoc.

[ok2c]

@twarner-sofi While PlainConnectionSocketFactory is non final and can be extended, one must be aware of risks of tight coupling through inheritance and risks of subclassing a concrete class and overriding methods that were not meant for extension in the first place. Do not pin this problem on us.

[twarner-sofi]

Okay, thanks for confirming that these methods are not meant for extension in the first place. It's unfortunate that they weren't marked final, or documented as such, but that's fine now.

We'll look into making a fix PR for docker-java.

[Sineaggi]

The check for the proxy being non null should be performed by the method.

There are two overloads of the method createSocket in the interface, one that takes a Proxy, the other one doesn't. I don't think it's obvious at first glance that the check for the proxy being non null should ever have to be performed by this method. The documentation for the method even says

via a proxy (generally SOCKS is expected)

which would imply that method is called via a proxy (i.e. the proxy is non-null).

[Sineaggi]

one must be aware of risks of tight coupling through inheritance and risks of subclassing a concrete class and overriding methods that were not meant for extension in the first place

The class in question has no private fields and no hidden internal logic, and has existed virtually unmodified since its creation in 2012. Without warning, this public and non-final class has had a breaking change made to its behavior in a patch release of this library.

[ok2c]

@Sineaggi There is no breaking change. The class remains fully API compatible. Any particular modality or implementation details have never been a part of the public contract. You abused class inheritance in your code.

[ok2c]

@Sineaggi Same.

[ok2c]

As it stands, there's no way for createSocket(HttpContext to be called anymore

@Sineaggi So what? This method will get removed in the next major release. I do not see how this necessitates any additional checks at the higher level. Individual socket factories must be able to handle null proxy parameters gracefully.

[Sineaggi]

As it stands, there's no way for createSocket(HttpContext to be called anymore

@Sineaggi So what? This method will get removed in the next major release.

Where does it say that this method will be removed? I couldn't find anything in the javadocs or release notes.

Individual socket factories must be able to handle null proxy parameters gracefully.

Again, I couldn't find any documentation mentioning as such. Presently the wording would imply createSocket that doesn't take a proxy parameter would be called if there was no proxy, and the new method would be called if there is a proxy.

[Sineaggi]

If going forward the design of the connection factory class ought to use the new createSocket(proxy, context) method, for the sake of existing users of the library would you be alright with modifying the existing overloads of createSocket in SSL and PlainConnectionFactory to call the previous createSocket method on null?

For example, like the following

httpclient5/src/main/java/org/apache/hc/client5/http/ssl/SSLConnectionSocketFactory.java

return proxy != null ? new Socket(proxy) : createSocket(context);

[ok2c]

would you be alright with modifying the existing overloads of createSocket in SSL and PlainConnectionFactory to call the previous createSocket method on null?

@Sineaggi Sure, I would. Feel free to raise a PR with the proposed changes.

[Sineaggi]

@ok2c Here's the PR #536 with the proposed changes.