-
Notifications
You must be signed in to change notification settings - Fork 9k
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
Comments
cc @webron, would appreciate a priority assignment |
This is strange! |
Here is my test: |
a quick fix could be not to display such a large responses, instead show something like github does on diff: 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: |
Profile-20171028T214335.zip 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);
} |
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. |
@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 |
That's perfect then. |
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. |
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 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: 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
|
I was looking at the formatXml function and that code smells funny...
It might be late and I'm missing something ... |
@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 😄 |
I think we still have a problem with large responses, my test: I think we should still set a hard MAX for the response size and provide the download link for those large ones |
@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): |
why not render asynchronously in stages, like eg. postman does. |
btw this issue got much worse in recent versions, presumably due to pretty-print / color enhancements which make rendering even more expensive. |
What's the purpose of showing/render a response larger than 2MB? |
your argument makes no sense to me. download is okay, it would resolve the most important aspect of this issue. |
here are a couple of ideas why someone would download:
Hopefully, that makes the argument make sense to you |
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. |
@plachor & @taranlu-houzz ...You might have missed my previous comment: ... consider contributing to this project:
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 |
... 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. |
@spyro2000 That is a Chrome extension I don't think that can be integrated into Swagger-UI... |
... and if any one wants to implement the idea to deactivate pretty-printing if response is greater than X, start here: |
Thanks for the hint on where to start, @heldersepu!
|
@kristiansamuelsson way to go!
Maybe that inspires others |
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):
I also tried adding the limit to other components, ex. The memory usage is still high when just expanding the operation, and even when disabling rendering for some of the components ( |
@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: Just expanding those crashes the browser, I didn't even get to try it out button |
Hello, We are also experiencing this issue. After investigating, I've found that Is there any possibility of replacing this component? I conducted a preliminary test by swapping it out for I'm ready to submit a pull request if the maintainers are open to this change. |
@nojaf 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, ...) |
@nojaf no worries project still active, in my experience any PR fixes a known issue will be accepted... https://www.npmjs.com/package/@monaco-editor/react https://www.npmjs.com/package/monaco-editor From your preliminary work looks you have it working... in your environment how much bugger did the |
@heldersepu thanks, yes, that is a fair take, I ran I'm not sure if that answers your question. Let me know if I need to run a different command. |
@nojaf that is great, a final bundle of |
Pah, just leave it broken and crashing for another few years and enjoy the 150 kB saved each time. Priorities! |
Ok @heldersepu, I went ahead and raised #9728. |
Anyone interested in testing the alpha prototype from @nojaf go to: This is your opportunity to influence how that will look like |
app.UseSwaggerUI(config => this improve performance |
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 1Give ability to disable syntax highlighting. As mentioned multiple times in this issue, syntax highlighting can be disabled fully by utilizing Step 2Detect possible large response bodies and disable syntax highlighting for them even when Step 3Detect 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 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. 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 |
Hi, can I ask how it is possible to set |
Is this still in progress? We have several endpoints that stream a large amount of data that developers from other teams want to test in swagger before setting up a rest client in their apps to hit our endpoints, but they've lost confidence in our application because they interpret Swagger hanging as our application hanging. I've tried directing them to curl the endpoints to see the true performance but it's not as convenient as swagger and they don't do it. |
AFAIK this has never been fixed. At least not internally. :( |
@jcjolley do you have a sample endpoint of your api we can use to test? |
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.The text was updated successfully, but these errors were encountered: