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

Slow handling response of large binary files (application/octet format) #394

Open
rubyshamir opened this issue Oct 22, 2018 · 4 comments
Open

Comments

@rubyshamir
Copy link

Hello and thanks for this great software!

We have a swagger definition containing a "GET" command returning a large file (format: "application/octet-stream").

The current implementation of response() eventually calls "unmarshal_response_inner" in "http_future.py". This method does not have a specific handling for binary data and encodes the data to string.

On large files, like in our case, this is a very time consuming process (2-3 seconds per 0.5 MB).
We have locally altered the code to return the binary data in case of "application/octet-stream" format and the time was reduced to ~0.1 seconds per 0.5 MB.

The altered function is as follows:

def unmarshal_response_inner(response, op):    
    ...
    # Code we added ##
    if content_type.startswith('application/octet'):
        return response.raw_bytes

    # TODO: Non-json response contents
    return response.text

We will be glad to learn if there is another workaround without changing the code.

Thanks in advance,

Ruby

@gp-greg
Copy link
Contributor

gp-greg commented Feb 11, 2019

We also encountered this scenario recently and implemented a similar workaround. Would you accept a PR with a change like Ruby has suggested? In our case we are working with Excel files so our content type is application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.

@macisamuele
Copy link
Collaborator

@greenphire-greg PRs are always welcome 😉

I'm still unsure about what should be the more appropriate return value in case of a response is not json or msg-pack.

@gp-greg
Copy link
Contributor

gp-greg commented Feb 11, 2019

I was hesitant to suggest changing the default return value too. I made a PR in the bravado-core repo, which if accepted I believe will need to be re-implemented here as well? (The distinction between bravado and bravado-core is not totally clear to me.)

Yelp/bravado-core#317

@advance512
Copy link

Would be nicer to also do this for image/, audio/, video/* responses.

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

4 participants