Heap space issue with queue

Question

I need some help with this code which is giving me a heap space issue. I have tried executors too but still I get out of memory issue.

The consists of a queue which initially holds an endpoint. The endpoint gives a response which has child endpoints which is again added to the queue. The response of the endpoint is also written to a file. For every 500 responses written, we change the file.

This is the code:

import java.io.BufferedReader;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.HttpURLConnection;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Queue;
import java.util.stream.Collectors;

import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;

import org.xml.sax.SAXException;

public class Hierarchy {

    private static Queue<String> linksQueue = new LinkedList<>();
    private static Queue<String> errorQueue = new LinkedList<>();
    private static HttpURLConnection connection;
    private static URL url;
    private static SAXParserFactory factory = SAXParserFactory.newInstance();
    private static SAXParser saxParser;
    private static XMLHandler xmlHandler = new XMLHandler();
    private static FileWriter writer;
    private static List<String> urlList = new ArrayList<>();
    private static int recordsPerFile = 500;
    private static int recordCount = 1;
    private static int fileCount = 1;
    private static String baseUrl = "parent url here";

    public static void main(String[] args) {
        String link = null;

        try {
            saxParser = factory.newSAXParser();
            writer = new FileWriter(new File("hierarchy/hierarchy_" + fileCount + ".txt"));
            linksQueue.add(baseUrl);

            while (!linksQueue.isEmpty()) {
                System.out.println("Size:: " + linksQueue.size());

                link = linksQueue.poll();
                if (link != null) {
                    System.out.println(link);
                    linksQueue.addAll(getChildLinks(link));
                }
                if (recordCount == recordsPerFile) {
                    doCleanUpForNextFile();
                }
            }
            writer.close();
        } catch (Exception e) {
            e.printStackTrace();
        }

    }

    private static void doCleanUpForNextFile() throws IOException, URISyntaxException {
        Files.write(Paths.get(new URI("error.txt").toString()), errorQueue, StandardOpenOption.APPEND);
        fileCount++;
        writer = new FileWriter(new File("hierarchy/hierarchy_" + fileCount + ".txt"));
        recordCount = 0;
    }

    private static List<String> getChildLinks(String link) {
        try {
            url = new URL(link);
            connection = (HttpURLConnection) url.openConnection();
            connection.connect();

            String result = new BufferedReader(new InputStreamReader(connection.getInputStream())).lines()
                    .collect(Collectors.joining());

            saxParser.parse(new ByteArrayInputStream(result.getBytes()), xmlHandler);
            urlList = xmlHandler.getURLList();

            writer.write(result + System.lineSeparator());
            recordCount++;
        } catch (IOException ioe) {
            ioe.printStackTrace();
            errorQueue.add(link);
        } catch (SAXException e) {
            e.printStackTrace();
            errorQueue.add(link);
        }

        connection.disconnect();
        return urlList;
    }

}

This is the XML handler class :

import java.util.ArrayList;
import java.util.List;

import org.xml.sax.Attributes;
import org.xml.sax.SAXException;
import org.xml.sax.helpers.DefaultHandler;

public class XMLHandler extends DefaultHandler {

    private List<String> urlList = new ArrayList<String>();

    @Override
    public void startElement(String uri, String localName, String qName, Attributes attributes) throws SAXException {
        if (qName.equalsIgnoreCase("link") && attributes.getValue(0).equalsIgnoreCase("child")) {
            if (attributes.getLength() >= 3) {
                this.urlList.add(attributes.getValue(2));
            } else {
                this.urlList.add(attributes.getValue(1));
            }
        }
    }

    @Override
    public void endElement(String uri, String localName, String qName) throws SAXException {
    }

    @Override
    public void characters(char ch[], int start, int length) throws SAXException {

    }

    public List<String> getURLList() {
        return urlList;
    }
}

The problem here is that the queue gets loaded with links at a very high rate while we read only one queue at a time. On average, for every one link polled from the queue, 500 links are added back again to it.

I took a heap dump and found that there are lot of string beings created with white spaces.

These are the issues that I face:

1. The queue gets full and heap space issue occurs. (I ran with 2GB of heapspace.)
2. The connection gets failed and throws an issue. (It occurs sometimes, don't know what the exact issue is. Will update here.)
3. Heap space issue due to something else.

I have been facing with this issue for 1 week now and still can't get to any solution or make this code run without any issues. Please help me to refine this code and make it run without any issues.

Answer 1

Instead of directly solving your issue, I'll actually review your code for now. In the process I'll probably touch on why you're running out of storage:

What "object oriented" means:

The whole point of using object-oriented programming languages is that you can separate and encapsulate data into instances of objects. This allows you to separate state of different execution contexts with very little effort.

It also helps to clarify "ownership semantics" on resources (like files, sockets, ...). This is one of the large issues I see in your code. So let's go fix that:

Avoid static

While this isn't generally good advice, in your case it's better to avoid static, since you're extremely overusing it.

A Hierarchy that you have should work completely differently from how it works right now:

Instead of calling main consumers of this class should have a dedicated API to attach to:

public class Hierarchy {

    public Hierarchy(String baseUrl) {
        this.baseUrl = baseUrl;
    }

    public void traverseLinks() {
        // this is where stuff from your main goes
    }
}

Now this is only a skeleton, but your main will look like this:

public static void main(String[] args) {
     new Hierarchy(baseUrl).traverseLinks();
}

To simplify your code and get it out of the static context, the first step is to push as many fields as possible down into methods, as well as making statics final and something akin to readonly for your methods:

The only static fields remaining are:

private static final SaxParser saxParser = SAXParserFactory.newInstance().newSAXParser();
private static final int DEFAULT_RECORDS_PER_FILE = 500;

Everything else is instance fields. This is especially valuable when you want to simultaneously traverse multiple hierarchies like so:

executor.submit(() -> new Hierarchy(baseUrl1).traverseLinks());
executor.submit(() -> new Hierarchy(baseUrl2).traverseLinks());

The next step is moving stuff that doesn't need to be a field away:

public void traverseLinks() {
    final Queue<String> linksQueue = new LinkedList<>();
    linksQueue.add(this.baseUrl);

    // this changes later
    FileWriter writer = new FileWriter(new File("hierarchy/hierarcy_" + fileCount + ".txt"));
    while (!linksQueue.isEmpty()) {
        String currentLink = linksQueue.poll();
        if (currentLink != null) {
            List<String> childLinks = getChildLinks(currentLink);
            // notice the difference:
            childLinks.stream().map(s -> s + System.lineSeparator()).forEach(writer::write);
            linksQueue.addAll(childLinks);
        }
        // we'll change this, so I left it out here
     }
     writer.close(); // or sth.
}

// throwing Exception here to keep compilable, but don't do that
// I'm making an exception (badumm, tss), because this is only intermediary
private List<String> getChildLinks(String link) throws Exception {
    final URL url = new URL(link)
    final HttpURLConnection connection = url.openConnection();
    connection.connect();

    // will also change later...
    String result = new BufferedReader(new InputStreamReader(connection.getInputStream())).lines()
                .collect(Collectors.joining());

    // no further changes in this method

At this point I'm somewhat okay with how the responsibities are shared across methods. Now it's time to talk about actually extracting responsibilities into separate classes:

Your problem has a few different requirements. In general it's useful to first get it to work correctly and then extract these requirements into well-specified classes, but that just makes things complicated here. While we're at that, let's talk about the bugs in your code:

• As mentioned by slowy: your XMLHandler implementation does not clear the urlList at any point in time.
• Your "rolling file" (the change of files after n records) is completely and utterly broken:
  If inside of getChildLinks you pass over the recordsPerFile threshold, then your file only gets switched ... effectively never. I strongly suspect you're not going through 5 million links, right? The problem is that the responsibility of counting (and writing) records is separated from the responsibility of switching the file after a certain number of records.

This latter one is what we'll fix next.

Here we extract a separate class, responsible for actually writing the links to the output:

public class RollingFileAppender implements AutoCloseable {
    private final String nameTemplate;
    private final Path basePath;

    private int currentCount;
    private final int switchThreshold;
    private int currentFileNumber;
    private Writer writer;

    public RollingFileAppender(Path basePath, String nameTemplate) {
        this.switchThreshold = 500; // can be customizable
        this.nameTemplate = nameTemplate;
        this.basePath = basePath; // notice I'm using Path and not file
        currentCount = 0;
        currentFileNumber = 1; // start at 1 because puny humans start there :)
        writer = new BufferedWriter(new OutputStreamWriter(
            Files.newOutputStream(basePath.resolve(String.format(nameTemplate, currentFileNumber)))
          ));
    }

    public void writeRecord(String currentRecord) {
        // TODO: Exercise: implement this **correctly**
    }

    @Override
    public void close() throws Exception() {
        // TODO: Exercise: implement this (really not that hard ;)
    }
}

---

Now that we got that separated out, we can refocus how traverseLinks works. At this point I should talk about resource handling:

The code doesn't take care of any of it's resources. That's really bad. Instead of just sprinkling a random close here and there and hoping you're cleaning everything up alright enough, you should get into the habit of using try-with-resources (introduced in Java 7).

That takes care of the resource handling for you. So we can now simplify to:

try (RollingFileAppender recordKeeper = new RollingFileAppender(Paths.get("hierarchy"), "hierarchy_%d.txt")) {
    while (!linksQueue.isEmpty()) {
        currentLink = linksQueue.poll();
        if (currentLink != null) {
            List<String> childLinks = getChildLinks(currentLink);
            childLinks.forEach(recordKeeper::writeRecord);
            linksQueue.addAll(childLinks);
        }
    }
} catch (Exception e) {
    // FIXME: catch specific exceptions, as close as possible 
    // to where they are thrown and if possible remove this
}
// recordKeeper is closed at this point

The last steps remaining now for this to be in a state where you can consider attempting to reduce memory footprint are:

• Finish implementation of RollingFileAppender
• Fix getChildLinks to not throw exceptions (tip: return a new List instance every time so you can throw it away in traverseLinks
• Fix XMLHandler to correctly clear urlList (tip: instances of this class are cheap)

Last but not least:

I feel kinda bad for bashing the code like this. It's pretty good code from a readability point of view. You chose good variable names and followed good practices for handling XML. The one thing that really broke your back here is the overuse of static. Then comes the misuse of Resources.

I think you can very much improve your coding by rereading a bit on how object orientation works and how to handle resources.

Answer 2

According to the numbers you provide, it just might be that you are trying to hold too mauch data in memory. Perhaps you need to persist the queue. There are many solutions for persisting a queue. Simply googling "java persistent queue" will give you many options.

Another popular solution is to use some no-sql database as replacement for the queue. why no-sql and not rdbms? no-sql is better suited for high load apps and scales better with workload