Java constant reference causes memory leak？

Question

jvm jstat command：

  jstat -gcutil 14378 2000 20
  S0     S1     E      O      M     CCS    YGC     YGCT    FGC    FGCT     GCT   
  8.04   0.00  87.61   9.06  96.45  94.34    702   13.804     8    2.521   16.325

Eclipse Memory Analysis Navigate to a code block：

public class MicrosoftConstant {

/**
 * TODO TTS 请求头设置
 */
public static final List<Header> TTS_REQUEST_HEADERS = new ArrayList<Header>(){
    {
        add(new BasicHeader("Content-Type", "application/ssml+xml"));
        add(new BasicHeader("X-Microsoft-OutputFormat", "xxx"));
        add(new BasicHeader("X-Search-AppId", "xxx"));
        add(new BasicHeader("X-Search-ClientID", "xxx"));
        add(new BasicHeader("User-Agent", "xxx"));
        add(new BasicHeader("Accept", "*/*"));
    }
};

}

Constants using code blocks：

List<Header> headers = MicrosoftConstant.TTS_REQUEST_HEADERS;
    headers.add(new BasicHeader("Ocp-Apim-Subscription-Key", microsoftConfig.getAppKey()));
    headers.add(new BasicHeader("Authorization", "Bearer " + authToken));
    InputStream audioStream = null;

        HttpEntity httpEntity = httpApiService.doPost(microsoftConfig.getTtsUrl(), body.getBytes(), headers);

The interface does not release memory during a large number of accesses。

I have no idea how to solve this problem. Can anyone provide a solution?

Mark Rotteveel · Accepted Answer · 2020-03-26T09:13:49.513

You not only have a memory leak, but also a possible security leak. The problem is that for each request you do, you add headers to the list TTS_REQUEST_HEADERS. This means that with each request, the list grows, and it never shrinks.

In addition, you use the 'double brace initialization' anti-pattern, but in this case that isn't that much of a problem.

Even worse, it means that the actual request may repeat certain headers multiple times (this depends on what the HTTP client does with repeated headers), which could unintentionally leak information about previous requests.

The solution to this problem is to copy the list, add your request specific headers to the copy, and use the copy to execute the request. To be sure you don't unintentionally modify the list in the constant, make sure it is not modifiable (that way it is actually a constant).

To do this, define the list as an unmodifiable list, for example:

public static final List<Header> TTS_REQUEST_HEADERS = 
        Collections.unmodifiableList(Arrays.asList(
                new BasicHeader("Content-Type", "application/ssml+xml"),
                new BasicHeader("X-Microsoft-OutputFormat", "xxx"),
                new BasicHeader("X-Search-AppId", "xxx"),
                new BasicHeader("X-Search-ClientID", "xxx"),
                new BasicHeader("User-Agent", "xxx"),
                new BasicHeader("Accept", "*/*")));

Or, for Java 9 and higher using List.of:

public static final List<Header> TTS_REQUEST_HEADERS = List.of(
        new BasicHeader("Content-Type", "application/ssml+xml"),
        new BasicHeader("X-Microsoft-OutputFormat", "xxx"),
        new BasicHeader("X-Search-AppId", "xxx"),
        new BasicHeader("X-Search-ClientID", "xxx"),
        new BasicHeader("User-Agent", "xxx"),
        new BasicHeader("Accept", "*/*"));

Your request code would then become:

// Copy the standard list of headers for this request
List<Header> headers = new ArrayList<>(MicrosoftConstant.TTS_REQUEST_HEADERS);
headers.add(new BasicHeader("Ocp-Apim-Subscription-Key", microsoftConfig.getAppKey()));
headers.add(new BasicHeader("Authorization", "Bearer " + authToken));
InputStream audioStream = null;

HttpEntity httpEntity = httpApiService.doPost(microsoftConfig.getTtsUrl(), body.getBytes(), headers);

This is a better solution than the one I propose below. I'm not too familiar with the new unmodifiableList collection functions. — claj, Mar 26 '20 at 09:09
@claj `Collections.unmodifiableList` wraps an existing list and disallows modification through that wrapper (so the underlying list cannot be modified through the wrapper, but could be modified by parts of the program with direct access to that list). Java 9's `List.of` produces an immutable list. — Mark Rotteveel, Mar 26 '20 at 09:13

score 0 · Answer 2 · answered Mar 26 '20 at 09:07

0

Every time you run the second code block, the TTS_REQUEST_HEADERS gets two more items in it. That means that for each request there are two more headers added. The TTS_REQUEST_HEADERS object's data is not constant with final (just pointer to the object is final and cannot be changed, the object itself can change, the list can grow) and the .add method will make it longer and longer.

You could .clone the TTS_REQUEST_HEADERS before adding the who headers for each specific request. It is a shallow copy, which means the BasicHeaders you add in the TTS_REQUEST_HEADERS initially, will be re-used.

Maybe you will have to change public static final List<Header> to public static final ArrayList<Header> to get .clone to work.

answered Mar 26 '20 at 09:07

claj

5,172
2
27
30

Using `clone()` is not a good idea, using `new ArrayList<>()` is better. The `TTS_REQUEST_HEADERS` shouldn't be modifiable, so the collection needs to be immutable (or at least not modifiable through this reference). And cloning an immutable list would result in an immutable list, which would prevent adding new entries. – Mark Rotteveel Mar 26 '20 at 09:09
Cloning would work just fine. It's implementation does create a new ArrayList and puts the pointers to the items from the original list in the new list. No modification of the original list is possible. But if you change the objects that is referenced by the original list it's bad... – claj Mar 26 '20 at 09:12
Yes, cloning will work fine with `ArrayList`, but you want to prevent modification of the list in the constant (otherwise it wouldn't be a constant) to prevent programming errors like the OP has now. And preventing modification would disallow cloning (or at least, make cloning not very useful because a clone - if possible - of an unmodifiable list should be unmodifiable as well). – Mark Rotteveel Mar 26 '20 at 09:15
I fully agree. The convention that a constant list must be cloned is really a bad one. An immutable list preferable! – claj Mar 26 '20 at 09:17

Java constant reference causes memory leak？

2 Answers2