cachecontrol icon indicating copy to clipboard operation
cachecontrol copied to clipboard

fix streaming error "Unable to determine whether fp is closed."

Open AndCycle opened this issue 2 years ago • 5 comments

trying to fix bug with new SeparateBodyBaseCache,

hit this when I write a custom SQLite cache backend with streaming resp.

AndCycle avatar Aug 08 '22 23:08 AndCycle

  • urllib3/util/response.py
def is_fp_closed(obj):
    """
    Checks whether a given file-like object is closed.

    :param obj:
        The file-like object to check.
    """

    try:
        # Check `isclosed()` first, in case Python3 doesn't set `closed`.
        # GH Issue #928
        return obj.isclosed()
    except AttributeError:
        pass

    try:
        # Check via the official file-like-object way.
        return obj.closed
    except AttributeError:
        pass

    try:
        # Check if the object is a container for another file-like object that
        # gets released on exhaustion (e.g. HTTPResponse).
        return obj.fp is None
    except AttributeError:
        pass

    raise ValueError("Unable to determine whether fp is closed.")

AndCycle avatar Aug 09 '22 14:08 AndCycle

The comment above with code, and the pull request, don't really seem related, and it's also not clear what the bug is. So you can explain:

  1. What is the bug is that motivated the PR?
  2. What that comment is for?

itamarst avatar Sep 19 '22 11:09 itamarst

@itamarst I haven't finished the full trace what path involve the issue, I just dig out which part raise the exception,

as I wrote a custom sqlite backend which use SeparateBodyBaseCache as base class I return the body part as bytes, the streaming path require the body have file-like interface in order to properly function, the patch just a quick fix that wrap the body part into file-like obj if it's not one.

AndCycle avatar Sep 19 '22 17:09 AndCycle

  1. If you're returning a giant pile of bytes in one go, that doesn't give you the advantage of SeparateBodyBaseCache; you're still using a lot of memory. So I'm not sure you want to bother with that base class unless you're actually streaming?
  2. If you do want to use it like this anyway, wouldn't wrapping the bytes with BytesIO in your class make more sense, rather than patching CacheControl? The interface uses a file-like object for a reason.

itamarst avatar Sep 21 '22 13:09 itamarst

@itamarst I am not really working on a giant pile of bytes, just some MPEG-DASH,

this patch is a simple version to catch any incorrect implementation, we need a better document about what the interface should return or write better error handling to remind developer, not a ValueError exception hidden in another lib without any hint.

the original implement always cause a full write on HTTP 304 Not Modified, separate the header and body write reduce this side effect significantly, this is why I work on this.

AndCycle avatar Sep 21 '22 16:09 AndCycle