HttpComponents HttpClient
  1. HttpComponents HttpClient
  2. HTTPCLIENT-1157

MemcachedHttpCacheStorage should throw IOExceptions instead of Runtime Exceptions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.2 Alpha1
    • Fix Version/s: 4.1.3, 4.2 Beta1
    • Component/s: HttpCache
    • Labels:

      Description

      The MemcachedHttpCacheStorage class implements HttpCacheStorage which defines that methods will throw IOExceptions, but the underlying net.spy.memcached.MemcachedClientIF throws runtime exceptions. These exceptions are not caught in the code where IOExceptions are expected causing these exception bubble up to the calling code. It seems like the MemcachedHttpCacheStorage class should treat at least some of these runtime exceptions as IOExceptions so that normal code execution paths can be followed.

      I'm proposing that MemcachedHttpCacheStorage treat a OperationTimeoutException from the memcached client as an IOException. This would allow the existing CachingHttpClient code to catch and log the exception as a warning, instead of bubbling the exception up the calling code.

      1. httpclient-1157-2.patch
        6 kB
        James Miller
      2. HTTPCLIENT-1157.patch
        10 kB
        James Miller

        Activity

        Hide
        James Miller added a comment -

        Looks good, thanks!

        Show
        James Miller added a comment - Looks good, thanks!
        Hide
        Jon Moore added a comment -

        Backported to 4.1.x. @James: Please verify and close issue if appropriate. Thanks!

        Show
        Jon Moore added a comment - Backported to 4.1.x. @James: Please verify and close issue if appropriate. Thanks!
        Hide
        Oleg Kalnichevski added a comment -

        @Jon. My pleasure. Truth to be told I was not going to, but I needed the trunk to be compilable to be able to publish the latest snapshot.

        Oleg

        Show
        Oleg Kalnichevski added a comment - @Jon. My pleasure. Truth to be told I was not going to, but I needed the trunk to be compilable to be able to publish the latest snapshot. Oleg
        Hide
        Jon Moore added a comment -

        @Oleg: I see you beat me to it. Thanks.

        Show
        Jon Moore added a comment - @Oleg: I see you beat me to it. Thanks.
        Hide
        Jon Moore added a comment -

        Also agreed – my bad. I'll fix it when I get into the office this morning. Where is our virtual collection jar so I can put my $1 in for breaking the build?

        Show
        Jon Moore added a comment - Also agreed – my bad. I'll fix it when I get into the office this morning. Where is our virtual collection jar so I can put my $1 in for breaking the build?
        Hide
        Sebb added a comment -

        Agreed.

        If we do decide to move to 1.6 minimum, it should be on the basis of some functionality that is only available in Java 1.6.

        Show
        Sebb added a comment - Agreed. If we do decide to move to 1.6 minimum, it should be on the basis of some functionality that is only available in Java 1.6.
        Hide
        Oleg Kalnichevski added a comment -

        @Jon
        The latest changes in SVN trunk introduced dependency on a Java 1.6 specific IOexception constructor, which broke the build

        https://builds.apache.org/job/HttpComponents%20Client/402/console

        We need to keep HttpClient Java 1.5 compatible, at least for now.

        Oleg

        Show
        Oleg Kalnichevski added a comment - @Jon The latest changes in SVN trunk introduced dependency on a Java 1.6 specific IOexception constructor, which broke the build https://builds.apache.org/job/HttpComponents%20Client/402/console We need to keep HttpClient Java 1.5 compatible, at least for now. Oleg
        Hide
        Jon Moore added a comment -

        @James: patch committed to trunk. I also wrapped try...catch around the cache puts and cache deletes as well for completeness. Please take a look, and if that works for you I'll also backport to 4.1.x. Many thanks for this!

        Show
        Jon Moore added a comment - @James: patch committed to trunk. I also wrapped try...catch around the cache puts and cache deletes as well for completeness. Please take a look, and if that works for you I'll also backport to 4.1.x. Many thanks for this!
        Hide
        James Miller added a comment -

        Here is an updated patch that only wraps the MemcachedClientIF methods that throw a OperationTimeoutException in a try..catch and throws an IOException instead

        Show
        James Miller added a comment - Here is an updated patch that only wraps the MemcachedClientIF methods that throw a OperationTimeoutException in a try..catch and throws an IOException instead
        Hide
        Jon Moore added a comment -

        @James: yes, I think just putting the try...catch around the Memcached operations would be overall simpler. If you can update the patch I'd be happy to apply it.

        Show
        Jon Moore added a comment - @James: yes, I think just putting the try...catch around the Memcached operations would be overall simpler. If you can update the patch I'd be happy to apply it.
        Hide
        James Miller added a comment -

        @Jon: I started out that way, but I felt that all the try..catch blocks distracted from actual cache logic. I can update the patch if you feel that is cleaner.

        Show
        James Miller added a comment - @Jon: I started out that way, but I felt that all the try..catch blocks distracted from actual cache logic. I can update the patch if you feel that is cleaner.
        Hide
        Jon Moore added a comment -

        @James: why not just wrap some try...catch blocks around the current calls to the MemcachedClientIF in MemcachedHttpCacheStorage? You can catch the OperationTimeoutException and re-throw wrapped in an IOException.

        Show
        Jon Moore added a comment - @James: why not just wrap some try...catch blocks around the current calls to the MemcachedClientIF in MemcachedHttpCacheStorage? You can catch the OperationTimeoutException and re-throw wrapped in an IOException.
        Hide
        Oleg Kalnichevski added a comment -

        If no one else steps in, I'll commit the patch in a few days

        Oleg

        Show
        Oleg Kalnichevski added a comment - If no one else steps in, I'll commit the patch in a few days Oleg
        Hide
        James Miller added a comment -

        Here is a patch wrapping the OperationTimeoutException as an IOException so that the normal code path can be followed.

        Show
        James Miller added a comment - Here is a patch wrapping the OperationTimeoutException as an IOException so that the normal code path can be followed.

          People

          • Assignee:
            Jon Moore
            Reporter:
            James Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development