Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MultipartEncoder can't handle mmap.mmap object correctly #346

Open
yx1994 opened this issue Jan 2, 2023 · 3 comments
Open

MultipartEncoder can't handle mmap.mmap object correctly #346

yx1994 opened this issue Jan 2, 2023 · 3 comments

Comments

@yx1994
Copy link

yx1994 commented Jan 2, 2023

MultipartEncoder can't handle mmap.mmap object correctly.

MultipartEncoder can't handle mmap.mmap object correctly. For example,

from requests_toolbelt import MultipartEncoder
import mmap

m = MultipartEncoder(
    fields={
            'field': ('filename', mmap.mmap(-1, 10))}
    )

m.read()

it will fall into an infinity loop in m.read().

I review those source code, find that the coerce_data function did't consider about the mmap.mmap object. Then the total_len function work different from other io object (always return the origin size). Then the bytes_left_to_write function always return True, fall into the infinity loop.

In my opinion, judge the mmap.mmap object by isinstance(data, mmap.mmap) or hasattr(data, 'madvise'), or other method?

Now, I fix my code by manually wrapped with FileWrapper, as

from requests_toolbelt.multipart.encoder import MultipartEncoder, FileWrapper
import mmap

m = MultipartEncoder(
    fields={
            'field': ('filename', FileWrapper(mmap.mmap(-1, 10)))}
    )

m.read()

I have little experience to pull requests. Could someone fix it from this package?

@sigmavirus24
Copy link
Collaborator

Nothing considered it because it wasn't intended to be supported. We could more explicitly error on this, @pquentin

@pquentin
Copy link
Contributor

Well, if wrapping it as a FileWrapper works maybe we could do that automatically in coerce_data?

@sigmavirus24
Copy link
Collaborator

If we land this for urllib3, sure. I'm not convinced this library needs feature work, even if it's low hanging fruit like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants