Uploaded image for project: 'HttpComponents HttpClient'
  1. HttpComponents HttpClient
  2. HTTPCLIENT-2305

Proposal: SSLConnectionSocketFactory refactor to allow custom timer metrics on socket.connect

    XMLWordPrintableJSON

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • 5.2.1
    • 5.3
    • HttpClient (classic)
    • None

    Description

      I'd like to add a timer metric around socket.connect in one of my classic blocking clients.

      Usually, I can implement such a thing by subclassing or introducing a delagating implementation, adding a small piece of code to record metrics before delegating to the original implementation – this works incredibly well for PoolingHttpClientConnectionManager here, for example, which covers the full connect + handshake process across all ipaddrs resolved for the provided hostname.

      Unfortunately, it's quite a bit more challenging to implement similar metrics over individual `Socket.connect` calls because they occur within a single large and complex method in SSLConnectionSocketFactory, which must delegate to other private methods: https://github.com/apache/httpcomponents-client/blob/19f3922b37369431d379ce57fa187740b22436cf/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/SSLConnectionSocketFactory.java#L223-L266

      Proposal:

      I would like to introduce a new method along these lines:

      void connectSocket(
          Socket sock,
          Timeout connectTimeout,
          HttpContext context)
          throws IOException
      

      This way I can override the method and delegate to the builtin implementation, while timing only the socket.connect component of connection establishment.

      Open questions:
      1. Would you accept such a change?
      2. The proposed example above uses the 'connectSocket' method name which would be overloaded with existing methods on ConnectionSocketFactory. Perhaps a more specific name like 'connectSocketTo' would be helpful here? Either way, I would need to provide javadoc describing the guarantee that only 'socket.connect' is handled.
      3. Should such a new method be added as a default interface method on ConnectionSocketFactory, or a new protected method on the SSLConnectionSocketFactory implementation? Updating the implementation is the simpler change, but providing the method on the interface may be more extensible.

      Feedback is always appreciated, thanks!

      Attachments

        Activity

          People

            ckozak Carter Kozak
            ckozak Carter Kozak
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 40m
                40m