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

Large response bodies cause hanging #3832

Open
shockey opened this issue Oct 28, 2017 · 91 comments · Fixed by #3862 · May be fixed by #9625
Open

Large response bodies cause hanging #3832

shockey opened this issue Oct 28, 2017 · 91 comments · Fixed by #3862 · May be fixed by #9625

Comments

@shockey
Copy link
Contributor

shockey commented Oct 28, 2017

Q A
Bug or feature request? Bug
Which Swagger/OpenAPI version? 2.0
Which Swagger-UI version? 3.4.0
How did you install Swagger-UI? local dev server
Which browser & version? Chrome 61
Which operating system? macOS High Sierra

Demonstration API definition

Swagger 2 Petstore is sufficient.

Configuration (browser query string, constructor, config.yaml)

Default.

Current Behavior

As an example, GET /pet/findByStatus in the Petstore currently returns 1.8MB of JSON when all statuses are selected. This is a pretty big chunk of data, but Swagger-UI insists on rendering the entire body, which is neither performant or useful. The main thread of the application is locked for upwards of 20 seconds on my machine.

Possible Solution

Truncate or refuse to display large textual response bodies.

Context

GET /pet/findByStatus is my favorite operation to use when testing UI enums, so this is something that impedes me pretty regularly.

@shockey
Copy link
Contributor Author

shockey commented Oct 28, 2017

cc @webron, would appreciate a priority assignment

@heldersepu
Copy link
Contributor

This is strange!
I swear I've used larger responses without any issues, let me see if I can make one up...

@heldersepu
Copy link
Contributor

heldersepu commented Oct 28, 2017

Here is my test:
http://swashbuckletest.azurewebsites.net/swagger/ui/index?filter=Huge#/HugeResponse/HugeResponse_Get
Yes we have a problem, even with 100000 it generates a 378K response that takes way too long

@heldersepu
Copy link
Contributor

heldersepu commented Oct 28, 2017

a quick fix could be not to display such a large responses, instead show something like github does on diff:
Large diffs are not rendered by default.

or truncate the body to 20000:

<ResponseBody content={ body.substring(0,20000) }
        contentType={ contentType }
        url={ url }
        headers={ headers }
        getComponent={ getComponent }/>

If we truncate the response will show: can't parse JSON. Raw result:

@heldersepu
Copy link
Contributor

Profile-20171028T214335.zip
Attached is my performance profile I hope that can help troubleshooting.
something in react is taking too long
webpack:///./~/react-dom/lib/DOMChildrenOperations.js?568f

function removeChild(parentNode, childNode) {
  if (Array.isArray(childNode)) {
    var closingComment = childNode[1];
    childNode = childNode[0];
    removeDelimitedText(parentNode, childNode, closingComment);
    parentNode.removeChild(closingComment);
  }
  parentNode.removeChild(childNode);
}

@webron
Copy link
Contributor

webron commented Oct 30, 2017

As @heldersepu suggested, I believe it's fair to limit the size that's being rendered, and if it's larger, make a link to download the result separately. This is not perfect though, as it means running the call again, and for some APIs it won't necessarily return the same result.

@shockey
Copy link
Contributor Author

shockey commented Oct 30, 2017

@webron, we'd be able to store the response string we already received, and allow the user to download it as a file - no second request needed.

As a proof of concept, see the Save as JSON/Save as YAML options in Swagger-Editor: we take the current editor content as a string and download it as a file, all from the client side.

@webron
Copy link
Contributor

webron commented Oct 30, 2017

That's perfect then.

@heldersepu
Copy link
Contributor

heldersepu commented Oct 30, 2017

@shockey & @webron Love the solution! the download link will be awesome!

...but someone should try looking into why the slowdown
You guys don't know any react guru that can truly get to the bottom of the issue?

@webron
Copy link
Contributor

webron commented Oct 30, 2017

In the past, we had issues with the syntax highlighter and large responses, but that doesn't seem to be the issue. I've tested the same API call in both Nightly and Chrome, and they both render fairly quickly (~3 seconds) when the format is JSON. When the format is XML though... that's when it explodes.

@shockey
Copy link
Contributor Author

shockey commented Nov 1, 2017

Okay, I dug into this a bit more, and @webron's note about XML is spot on.

When rendering XML, React spends a ton of time in the ResponseBody render function:

messages image 4167817450

Everything else looks performant, so the good news is that our problem is contained to one component.

Doing a traditional profile on call stack times between clicking Execute and seeing the body revealed the true issue:

messages image 880229148

More than 95% of the main thread's time is spent parsing the whitespace-stripped XML that comes from the server into a pretty™ XML string. Even more specifically, this line appears to be the bottleneck: https://github.com/swagger-api/swagger-ui/blob/master/src/core/utils.js#L222

TL;DR: it's the formatXml(xmlStr) prettifier function in utils.js

@heldersepu
Copy link
Contributor

I was looking at the formatXml function and that code smells funny...
Inside that function there is a section where we:

  • create an array types
  • loop over types populating results
  • finally get the first one of result

It might be late and I'm missing something ...
...or that can be simplified, I'm sending you guys a PR

@shockey
Copy link
Contributor Author

shockey commented Nov 2, 2017

@heldersepu, I agree - note that the code was originally taken from an SO answer.

I'd be in favor of using a third-party module to handle the formatting. In the meantime, I'll take a look at your PR 😄

@heldersepu
Copy link
Contributor

I think we still have a problem with large responses, my test:
http://swashbuckletest.azurewebsites.net/swagger/ui/index?filter=Huge#/HugeResponse/HugeResponse_Get
that is a json response and it gets uncomfortably slow to render...

I think we should still set a hard MAX for the response size and provide the download link for those large ones

@shockey
Copy link
Contributor Author

shockey commented Nov 2, 2017

@heldersepu, yeah, I see it with your example - the request comes back in under a second, but the rendering takes about ten seconds. I agree with the hard max, but I'd also like to continue looking at the underlying causes and try to address as we go.

Reopening.

Looks like the lion's share of that time (about 8 seconds) is React fiddling with the DOM. The response rendering causes 370(!!) DOM operations to be queued, and >90% of them appear to be caused by JumpToPath affecting ModelCollapse, which is unrelated to the response coming in as far as I can see.

This is a performance issue, but I'm inclined to keep this separated from our big performance ticket, since it's related to Try-It-Out, not initial rendering.

Below is a snapshot of React's DOM operation queue (the HighlightCode update with the response body isn't even on the first page):

@masc3d
Copy link

masc3d commented Aug 15, 2020

why not render asynchronously in stages, like eg. postman does.
it works well and there's barely any need for switches or limits.

@masc3d
Copy link

masc3d commented Aug 15, 2020

btw this issue got much worse in recent versions, presumably due to pretty-print / color enhancements which make rendering even more expensive.

@heldersepu
Copy link
Contributor

why not render asynchronously in stages, like eg. postman does.
it works well and there's barely any need for switches or limits.

What's the purpose of showing/render a response larger than 2MB?
That is certainly not humanly readable, my sample above on postman takes a few seconds too on the "formating" step...
To me what github does on diff is the way to go, let the users know the response is large and give them an option to download content or render at own risk

@masc3d
Copy link

masc3d commented Aug 16, 2020

What's the purpose of showing/render a response larger than 2MB?

your argument makes no sense to me.
if there's no purpose (in showing the content), why would you download it to eventually do exactly that.

download is okay, it would resolve the most important aspect of this issue.
but it's still less convenient.

@heldersepu
Copy link
Contributor

heldersepu commented Aug 16, 2020

here are a couple of ideas why someone would download:

  • to send it to someone else
  • to processes it with a tool

Hopefully, that makes the argument make sense to you

@masc3d
Copy link

masc3d commented Aug 16, 2020

if it makes sense to download it (very often) makes even more sense to view the content right in place.

just for quick checking a subset of a larger result, having to download, open in another editor and cleaning up afterwards are just an awful lot of unnecessary and tiring additional steps.

@krocofish
Copy link

Sorry , we have formulas and charts - and to show data correct we need all data ... We store data in tree hierarchy and we do not store row id in this structure. This is our world ... Our front is pretty good written - it help a bit and on back we have a lot of end points in modern approach. My regards, Aleksander

@heldersepu
Copy link
Contributor

Alternatively, if you don't have time to contribute but want this to work with large payloads, simply use the workaround posted in the previous comment from September 2000. I continue to use this workaround for older APIs I don't have time to rework and paginate.

100x 👍

Another alternative for those that want to see this resolved and don't like any of the existing workarounds, also don't want to spend the time troubleshooting and debugging this, consider hiring someone

@plachor
Copy link

plachor commented Jan 29, 2024

@plachor Can you provide an example of a case where pagination is not an option, I honestly have not come across it.

In the past few years everyone I talk to is moving toward a microservices architecture, smaller parts and browser makes many calls retrieving partial data... think google maps, instead of loading a huge image we have small map tiles and the browsers makes many requests and then combines them in a nice map

Let's try.

In one of my cases I expose Elasticsearch index mapping and configuration, another endpoint exposes node configuration. If I try to run them browser hangs for a minute.

So each time you have a large object to return which is needed complete to start analysis. Case in which paging would only add spare code on both sides. Or would more look like chunking.

There can be endpoint returning JSON documents created by end user in some envelope.

@taranlu-houzz
Copy link

Just to add to this: not all apps that use Swagger are production apps that have many users. I have have some internal tools that use FastAPI (which uses Swagger), and it is more convenient to return large json objects directly in some cases.

@heldersepu
Copy link
Contributor

@plachor & @taranlu-houzz ...You might have missed my previous comment:

... consider contributing to this project:

  • very least provide a link to the API that reproduces the problem,
  • provide some ideas of how to fix it
  • ideally spend some time troubleshooting and debugging the code

You don't like any of the existing workarounds, also don't want to spend the time troubleshooting and debugging this or don't know how to, consider hiring someone that can
shoutout to @kenjdavidson I'm sure he can fix this for you, go sponsor him:
https://github.com/sponsors/kenjdavidson

@spyro2000
Copy link

... or you just use a better JSON printer like https://chromewebstore.google.com/detail/json-formatter/bcjindcccaagfpapjjmafapmmgkkhgoa?pli=1

This pretty-prints even super-large JSON in 2 seconds while the same JSON kills Swagger for 10s of minutes.

Or, please, just deactivate pretty-printing at all if > 1MB or so, please.

Please.

@heldersepu
Copy link
Contributor

@spyro2000 That is a Chrome extension I don't think that can be integrated into Swagger-UI...
but go ahead give it try, let us know how it goes

@heldersepu
Copy link
Contributor

... and if any one wants to implement the idea to deactivate pretty-printing if response is greater than X, start here:
https://github.com/search?q=repo%3Aswagger-api%2Fswagger-ui%20canSyntaxHighlight&type=code
The canSyntaxHighlight is the variable that will turn on/off that

@kristiansamuelsson
Copy link

Thanks for the hint on where to start, @heldersepu!
I have implemented two new config options to allow for:

  • disabling rendering if payload size is over a specified number of bytes, and/or
  • disabling pretty-printing if a payload size is over a specified number of bytes

@heldersepu
Copy link
Contributor

@kristiansamuelsson way to go!
Looking at your profile that is your first PR on GitHub, congrats! ...
Do you mind sharing your experience,

  • how hard was to debug/code that?
  • how long did it take you?

Maybe that inspires others

@glowcloud
Copy link
Contributor

Hi everyone, just letting you know I’m investigating this issue and testing this PR proposing a fix with a specification from this issue.

Unfortunately, it seems like only adding this render limit is not enough. You can check it out with these steps (using #9625):

  1. Load the mentioned spec while setting a low render limit
  2. Navigate to CRUD - AGM Disease Annotations
  3. Click on the first PUT request
  4. Wait for it to load
  5. Click on try it out
  6. Wait for the site to crash

I also tried adding the limit to other components, ex. RequestBodyEditor, Curl, but it hasn't fixed the crash when going through the mentioned steps.

The memory usage is still high when just expanding the operation, and even when disabling rendering for some of the components (RequestBody, Response). I think that the issue might partially lie with immutable types, at least from checking the memory usage. I will be investigating this further.

@heldersepu
Copy link
Contributor

heldersepu commented Mar 7, 2024

@glowcloud I'm not sure I follow ... those steps have nothing to do with this issue and large response bodies

This is the spec from #9662:

https://petstore.swagger.io/?docExpansion=None&defaultModelsExpandDepth=None&url=https://gist.githubusercontent.com/heldersepu/ab37902d2f687d2c71aa1b36bb72ee48/raw/0f68906074dfbad5528a81132ae7975b6bf2ef75/openapi.yml#/CRUD%20-%20AGM%20Disease%20Annotations

Just expanding those crashes the browser, I didn't even get to try it out button

@nojaf
Copy link

nojaf commented Mar 20, 2024

Hello,

We are also experiencing this issue. After investigating, I've found that react-syntax-highlighter seems to be the primary cause of the bottleneck:

Screenshot 2024-03-20 110733

Is there any possibility of replacing this component? I conducted a preliminary test by swapping it out for @monaco-editor/react and immediately observed significantly improved performance with large results.

Screenshot 2024-03-20 113832

I'm ready to submit a pull request if the maintainers are open to this change.

@heldersepu
Copy link
Contributor

@nojaf You don't have to ask permission to submit a PR ...
now regarding @monaco-editor/react that seems to be a full editor not just syntax highlighter, maybe there are some feature that can be turned off

@nojaf
Copy link

nojaf commented Mar 20, 2024

You don't have to ask permission to submit a PR ...

No, it makes more sense to find out if the maintainers would ever consider merging it before I spent my time creating the PR.

The Monaco editor can be tweaked to appear very minimal (readonly mode, remove line numbers, side bar, ...)

@heldersepu
Copy link
Contributor

@nojaf no worries project still active, in my experience any PR fixes a known issue will be accepted...
it is great that The Monaco editor can be tweaked to appear very minimal, but my concern is size:

https://www.npmjs.com/package/@monaco-editor/react
Unpacked Size
152 kB. <- that is great if is truly only that

https://www.npmjs.com/package/monaco-editor
Unpacked Size
92.2 MB <- now this is on the large side

From your preliminary work looks you have it working... in your environment how much bugger did the bundle get https://github.com/swagger-api/swagger-ui/tree/master/dist ?

@nojaf
Copy link

nojaf commented Mar 20, 2024

@heldersepu thanks, yes, that is a fair take, I ran npm run build:bundle and got:

image

I'm not sure if that answers your question. Let me know if I need to run a different command.

@heldersepu
Copy link
Contributor

@nojaf that is great, a final bundle of 1.3 MiB is no significant change in size

@spyro2000
Copy link

Pah, just leave it broken and crashing for another few years and enjoy the 150 kB saved each time. Priorities!

@nojaf
Copy link

nojaf commented Mar 21, 2024

Ok @heldersepu, I went ahead and raised #9728.
There are definitely some open questions left. I appreciate your input and am willing to tweak things accordingly.

@heldersepu
Copy link
Contributor

Anyone interested in testing the alpha prototype from @nojaf go to:
https://raw.githack.com/nojaf/swagger-ui/large-bodies/dist/index.html#/

This is your opportunity to influence how that will look like

@syednomanforyou
Copy link

app.UseSwaggerUI(config =>
{
config.ConfigObject.AdditionalItems["syntaxHighlight"] = new Dictionary<string, object>
{
["activated"] = false
};
});

this improve performance

@char0n
Copy link
Member

char0n commented Apr 10, 2024

Hi everybody,

Let me first address the current state of this issue. Where we are and where we want to get.

Large response bodies are indeed causing issues. The issue with large response bodies starts with Immutable serialization into redux store and continues with rendering with or without syntax highlighting.

We will address this problems progressively in multiple steps.

Step 1

Give ability to disable syntax highlighting. As mentioned multiple times in this issue, syntax highlighting can be disabled fully by utilizing syntaxHighlight.activated=false option. This is already fully supported.

Step 2

Detect possible large response bodies and disable syntax highlighting for them even when syntaxHighlight.activated=true. There is a PR that actually comes pretty close to our idea: #9625

Step 3

Detect possible large response bodies and disable rendering for them, but provide ability to download. This way we'll avoid any possible operation performed on large respose bodies expect storing it in redux store. We still have to provide ability to download the large response body though. Again #9625 comes pretty close to our idea.


Entire syntax highlighting feature has been consolidated into single syntax-highlighting plugin in #9783. This makes it possible to completely override how syntax highlighting is done or switch the syntax highlighter entirely. We opted to use highlight.js for default syntax highlighter as it's more performant than prism.


@nojaf,

Thank you for your PR, be we will not be incorporating your changes into the core SwaggerUI. We already use custom build MonacoEditor in SwaggerEditor@5. We have to be very careful what dependencies we pull into our bundles and how they affect each other. Using MonacoEditor is a major arch decision, we'll rather provide progressive enhancement to this issue as described in previous steps.

The #9783 opened possibility to utilize custom syntax highlighting mechanism, and I've seen you've already transformed your PR into the plugin that other people can utilize if they choose to.


@heldersepu,

https://petstore.swagger.io/?docExpansion=None&defaultModelsExpandDepth=None&url=https://gist.githubusercontent.com/heldersepu/ab37902d2f687d2c71aa1b36bb72ee48/raw/0f68906074dfbad5528a81132ae7975b6bf2ef75/openapi.yml#/CRUD%20-%20AGM%20Disease%20Annotations

This particular definition according to my research, suffers with different issues not related to large response bodies. When expanding operation most time is spent in dereferencing (resolution) and JSON Schema sample generation. Only after that (when browser still hasn't crashed) the syntax highliting issue comes into light.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment