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.

Sign up for GitHub

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

Jump to bottom

Bootstrap page payload remains in ThreadLocal cache managed by JSoup #6464

Closed
m-emelchenkov opened this issue Sep 16, 2019 · 5 comments · Fixed by #6551
Closed

Bootstrap page payload remains in ThreadLocal cache managed by JSoup #6464

m-emelchenkov opened this issue Sep 16, 2019 · 5 comments · Fixed by #6551
Labels
bug investigation
Milestone
2.1.0.beta3

Comments

@m-emelchenkov
Copy link

m-emelchenkov commented Sep 16, 2019 •
edited

Hello! I got a memory leak. I will describe it below as detailed as possible, and also attach a demo project to reproduce it on your machine.

Vaadin 14.0.4, Spring Boot 2.1.7, Java 11.0.4, Tomcat/Jetty/Undertow, macOS High Sierra + Safari.

Let's take my-starter-project as a base.

application.properties:

vaadin.heartbeatInterval=5
server.servlet.session.timeout=60

MainView.java:

    private static String generateRandomString(java.util.Random rnd, int length){
        // Return some string of size `length`
		...
    }

@Route
@UIScope
@Push(transport = Transport.LONG_POLLING)
public class MainView extends VerticalLayout {
...

public MainView(@Autowired MessageBean bean) {
	Button button = new Button("Click me",
		e -> {
			Random rnd = new Random();
			for (int i = 0; i < 50; i++) {
				Div div = new Div();
				div.setText(generateRandomString(rnd, 1024 * 10));
				this.add(div);
			}
			Notification.show(bean.getMessage());
		});
	add(button);
}

The above code outputs to browser window 50 lines of 10Kb each, no issues. Now, let's replace the constructor's code.

public MainView(@Autowired MessageBean bean) {
	Random rnd = new Random();
	for (int i = 0; i < 50; i++) {
		Div div = new Div();
		div.setText(generateRandomString(rnd, 1024 * 10));
		this.add(div);
	}
}

The above code has an issue: a memory leak -- several objects org.apache.tomcat.utils.threads.TaskThread are never destroyed (even after MainView objects count and SpringVaadinSession objects count become 0 due to session expiration). It is not related to servlet container, I tried all three available in Spring Boot: Tomcat, Jetty and Undertow. I attached a screenshot. It contains a string with Push data of AtmospherePushConnection (start with <!doctype html>...).

Problem

In constructor I pass a lot of data to user's browser window (a large log file, instead of random string data as in this sample), so this memory leak steals a lot of memory at each page refresh and finally leads to OutOfMemory heap error.

my-starter-project.zip
@Legioth
Copy link
Member

Legioth commented Sep 18, 2019

Based on my investigation, this isn't a regular memory leak per se. Instead, this is a per-thread cache managed by JSoup that may under specific end up with quite much content. What it means is that memory use doesn't grow without bounds, but instead only based on the number of request-serving threads used by the application server.

JSoup caches instances of StringBuilder to reduce the number of instances that would have to be garbage collected. The cache works so that there is one instance per Java thread, managed using a ThreadLocal. This is handled in the method org.jsoup.helper.StringUtil.stringBuilder().

Vaadin uses JSoup to build the bootstrap html page contents. Since this is the last use of JSoup in a typical Vaadin request, the full bootstrap page will remain in a cached string builder until the next time some JSoup functionality that uses a string builder is run on the same thread.

The caching was reimplemented in jhy/jsoup#1059 because of issues similar to this and the fix was included in JSoup version 1.12.1. Vaadin 14 is using version 1.11.3.

I haven't done a full test run with the new JSoup version, but at least the basic stuff seems to work and the reported issue can no longer be observed if I update pom.xml to explicitly use the newer version:

<dependency>
    <groupId>org.jsoup</groupId>
    <artifactId>jsoup</artifactId>
    <version>1.12.1</version>
</dependency>

If we for some reason cannot update Vaadin to use the newer JSoup version, then we could as a workaround add a dummy call to StringUtil.stringBuilder() right after building the bootstrap html string in the end of BootstrapHandler.synchronizedHandleRequest and WebComponentBootstrapHandler.writeBootstrapPage. This would cause the previous contents of the cached string builder to be cleared so that memory can be reclaimed immediately.

@Legioth Legioth changed the title Memory leak in Spring Boot servlet container threads Bootstrap page payload remains in ThreadLocal cache managed by JSoup Sep 18, 2019
@m-emelchenkov
Copy link
Author

@Legioth Leif, you are great person! You conducted a very deep investigation and found the root of the issue. I express my deep gratitude to you.

I added the newer JSoup version in my-started-project, and no abandoned caches anymore. I will integrate this change to a working app, and will test it in production.

@caalador caalador moved this from Needs triage to P2 - Medium Priority in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) Sep 23, 2019
@m-emelchenkov
Copy link
Author

Operating the software with the new JSoup version for a week. Works well, no OutOfMemory errors anymore. Thanks again for your help!

Legioth added a commit that referenced this issue Sep 26, 2019
@Legioth
Update to the latest JSoup version
68797d8 
Fixes #6464
@Legioth Legioth mentioned this issue Sep 26, 2019
Merged
OLD Vaadin Flow bugs & maintenance (Vaadin 10+) automation moved this from P2 - Medium Priority to Closed - pending release Sep 26, 2019
ujoni pushed a commit that referenced this issue Sep 26, 2019
@Legioth @ujoni
Update to the latest JSoup version (#6551)
db1f36f 
Fixes #6464
ujoni pushed a commit that referenced this issue Sep 26, 2019
@Legioth
Update to the latest JSoup version (#6551)
b0da1e7 
Fixes #6464

(cherry picked from commit db1f36f)
@Legioth
Copy link
Member

Legioth commented Sep 26, 2019

Thanks for verifying that the new JSoup version seems to work. I've updated Vaadin to use the new version. We ill most likely not release this update in a maintenance release, but it will instead be included in Vaadin 14.1 which will be released within a couple of months.

@m-emelchenkov
Copy link
Author

@Legioth Thank you for informing me about your release plans!

@m-emelchenkov m-emelchenkov mentioned this issue Sep 26, 2019
Closed
ujoni pushed a commit that referenced this issue Sep 27, 2019
@Legioth @ujoni
Update to the latest JSoup version (#6551)
b6594c4 
Fixes #6464

(cherry picked from commit db1f36f)
@joheriks joheriks added this to the 2.1.0.beta2 milestone Oct 7, 2019
@caalador caalador moved this from Closed - pending release to Closed - released in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug investigation
Projects
None yet
Milestone
2.1.0.beta3
Development

Successfully merging a pull request may close this issue.

Update to the latest JSoup version vaadin/flow
4 participants
@Legioth @caalador @joheriks @m-emelchenkov