HttpComponents HttpClient
  1. HttpComponents HttpClient
  2. HTTPCLIENT-1298

Unable to shutdown executor service used by AsynchronousValidator

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.2.3
    • Fix Version/s: 4.3 Beta1
    • Component/s: HttpCache
    • Labels:
      None

      Description

      Currently there is no way to tell the CachingHttpClient to shutdown the executor service used by its AsynchronousValidator. This could lead to a resource leak, but probably only in cases when the threads weren't reclaimed by the thread pool. So only when it is actually processing tasks. As long as the thread pool isn't used, it won't create threads.

      From an application life-cycle point of view there should be a way to explicitly tell the HttpClient to shutdown and release all resources now regardless whether there are any outstanding validation requests or not.

      I have a patch against version 4.2.1 which in fact adds a shutdown() method to the HttpClient and the AsynchronousValidator. Today I saw, there is already a CloseableHttpClient. So do you need any contribution to fix this? Looks like there is already a plan for that.

      Btw: thanks for already supporting the background validation via the stale-while-revalidate header.

        Activity

        Hide
        Oleg Kalnichevski added a comment -

        We cannot add #shutdown method to the HttpClient interface, so it may not be easy to fix this issue in the 4.2.x branch. CloseableHttpClient in the trunk, however, can do extra cleanups beside the expected shutdown of the connection manager. Have a loot at the CachingHttpClientBuilder in the caching module and see how it registers an extra Closeable with the underlying CloseableHttpClient implementation to ensure automatic cleanup of the file cache when the client shuts down. You could do something very similar for the executor service used by its AsynchronousValidator.

        Oleg

        Show
        Oleg Kalnichevski added a comment - We cannot add #shutdown method to the HttpClient interface, so it may not be easy to fix this issue in the 4.2.x branch. CloseableHttpClient in the trunk, however, can do extra cleanups beside the expected shutdown of the connection manager. Have a loot at the CachingHttpClientBuilder in the caching module and see how it registers an extra Closeable with the underlying CloseableHttpClient implementation to ensure automatic cleanup of the file cache when the client shuts down. You could do something very similar for the executor service used by its AsynchronousValidator. Oleg
        Hide
        Martin Meinhold [Atlassian] added a comment -

        I understand that changing 4.2 is not an option, but that's ok. Getting the change into trunk is totally acceptable.

        The CachingHttpClientBuilder looks like the right place to do it. So there the AsynchronousValidator could be constructed and registered for later cleanup. IMHO this requires to break the constructor dependency circle between the CachingExec and the AsynchronousValidator by removing the CachingExec field from the validator and pass it as parameter to revalidateCacheEntry() method. If you agree with that, I can start coding and submit you a patch.

        Cheers,

        Martin

        Show
        Martin Meinhold [Atlassian] added a comment - I understand that changing 4.2 is not an option, but that's ok. Getting the change into trunk is totally acceptable. The CachingHttpClientBuilder looks like the right place to do it. So there the AsynchronousValidator could be constructed and registered for later cleanup. IMHO this requires to break the constructor dependency circle between the CachingExec and the AsynchronousValidator by removing the CachingExec field from the validator and pass it as parameter to revalidateCacheEntry() method. If you agree with that, I can start coding and submit you a patch. Cheers, Martin
        Hide
        Oleg Kalnichevski added a comment -

        Martin, that sounds like a plan. Please hack away!

        Oleg

        Show
        Oleg Kalnichevski added a comment - Martin, that sounds like a plan. Please hack away! Oleg
        Hide
        Martin Meinhold [Atlassian] added a comment -

        Let me know if you have any trouble with the patch.

        Show
        Martin Meinhold [Atlassian] added a comment - Let me know if you have any trouble with the patch.
        Hide
        Oleg Kalnichevski added a comment -

        Patch committed to SVN trunk. Many thanks, Martin, for contributing it.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Patch committed to SVN trunk. Many thanks, Martin, for contributing it. Oleg

          People

          • Assignee:
            Unassigned
            Reporter:
            Martin Meinhold [Atlassian]
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development