1
\$\begingroup\$

Awhile back, I wrote a quick and dirty webapp to read the remote server's log files, without a user needing to ssh in to view it via command line. It's meant to be a quick and dirty tool since we don't have anything like logstash available.

I am rewriting in Java 8 and have used the Streaming API to remove about 60% of the existing code.

What I need reviewed is the following function:

@RequestMapping(value="logLines") public void getLog(@RequestParam String logName, @RequestParam Integer logSize, HttpServletResponse response) throws Exception { log.debug("Entered getLog(). log: {}, logSize: {}", logName, logSize); PrintWriter writer = response.getWriter(); // stream a line at a time to try to minimize memory usage Stream<String> stream = logService.getLogStream(logName, logSize); stream.forEach(p -> { writer.println(p); }); } 

The previous version of the logService.getLogStream() method returned a List<String> which would have at least 1000 entries (and likely more). This version is meant to be optimized to avoid the memory overhead of a large List implementation. Also, it is meant so that the garbage collector can clean up used entries.

Am I really getting the benefits here that I want?

Here's the service method being used. There's some rudimentary HTML processing stuff being done to the lines read from the text file, which, if this wasn't quick and dirty code, I'd find a more elegant way to do.

/* JAVA 8 CODE */ @Override public Stream<String> getLogStream(String logName, int lines) throws Exception { log.debug("Entered getLogStream()"); final String filepath = properties.getProperty(logName); Path path = Paths.get(filepath); if (Files.exists(path)) { long countOfLines = Files.lines(path).count(); Stream<String> result = Files.lines(path) .skip(Math.max(0, countOfLines - lines)) .map(p -> p.trim()) .map(p -> HtmlUtils.htmlEscape(p)) // Spring Util to escape the string for HTML since I'm already using Spring .map(p -> { if (p.startsWith("at ")) { return "<span style=\"margin-left: 20px\" class=\"exceptionMessage\">" + p + "</span><br/>\r\n"; } else { return p + "<br/>"; } }); return result; } else { throw new IOException(String.format("Log file %s not found!", logName)); } } 
\$\endgroup\$
1

1 Answer 1

1
\$\begingroup\$

If you want to prevent the whole payload from being loaded into memory, you’ll very likely need to send a chunked http response as well. Otherwise I can’t see how the payload doesn’t need to be loaded into RAM completely at some stage.

Looking at the documentation for ServletResponse.html#getWriter(), it seems like the PrintWriter just accumulates all writes in memory until it is finally flushed (and the http response sent).

Another point to consider is that logService (whose code you did not supply) should of course not just load the file and then stream its lines, but needs to really stream the file from disk. Otherwise you’ll not have won anything.

Finally a small tip: In place of an inline lambda you can more concisely write stream.forEach(writer::println) in this case. :)

edit: Service looks fine, but you’re reading the whole file twice – once to find number of lines, and then a second time to get the actual output. (And the number of lines might even have changed in the meantime.) I can’t think of a good solution to this, sadly.

Btw, again I recommend using String::trim and HtmlUtils::htmlEscape as somewhat more elegant. You can also extract the third function into a private String addHtmlFormatting(String escapedError) and then reference it like this::addHtmlFormatting.

\$\endgroup\$
1
  • \$\begingroup\$ I added the service method in question since a coworker expressed some concerns as well. \$\endgroup\$ Commented Dec 9, 2016 at 13:40

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.