A callback that never returned anything useful, typed to say so

Background

CachingPortletOutputHandler (CachingPortletOutputHandler.java:30) captures a portlet's output for reuse. Its Javadoc (CachingPortletOutputHandler.java:26-29) says it caches up to maximumSize bytes or characters and gives up caching past that. PortletRendererImpl builds the handler with the portal's cache size threshold (PortletRendererImpl.java:434-436). That threshold, cacheSizeThreshold, defaults to 102400, which the code comment calls 100 KB (PortletCacheControlServiceImpl.java:73-74). A portal operator can override it through the cacheSizeThreshold property that sits commented out at portal.properties:383.

The handler does its capturing with a tee. When the portlet asks for a writer through getPrintWriter, the handler wraps the real response writer in a LimitingTeeWriter and makes an in-memory StringBuilderWriter, its cachingWriter, the branch. A portlet that asks for bytes through getOutputStream gets the same treatment from LimitingTeeOutputStream and a ByteArrayOutputStream. The limiting tee counts what passes through. Once the count goes over the limit, it swaps the branch for NullWriter.NULL_WRITER, a writer that discards everything, and it calls a callback held in a field named limitReachedCallback. The handler's callback empties the partial copy through clearCachedWriter() or clearCachedStream(). Later, the handler's getCachedPortletData checks each tee's isLimitReached() and returns nothing for a tee that hit its limit.

That callback is what this change retypes. Before PR 2943, the tee called this.limitReachedCallback.apply(this); and dropped the result. PR 2943, titled "Fixes for guava version bump", assigned the result to a local variable named unused, which its code comment says satisfies CheckReturnValue. It also put @SuppressWarnings("unused") on that local, so a variable nobody reads raises no warning of its own. PR 2945 removes that workaround and changes the type instead. Its body calls the Consumer version the cleaner design for the same problem.

The PR body also says why the change arrived as a separate pull request. According to the body, the Consumer version was first written as commit f2e3a459 on the remove_fluid branch, the branch behind PR 2915, and reached PR 2943's branch only after PR 2943 had merged. PR 2943's timeline shows its one commit, adcfd74, and the merge. A merged pull request no longer follows its branch, so that timeline cannot confirm the later push.

Intuition

The tee calls its callback for a side effect: empty the partial copy. It never reads anything back. PR 2943 kept a type that promises a result, then had to explain at the call site why the result was dropped. PR 2945 changes the type to one that promises no result, and the call site has nothing left to explain.

Take a toy limit of 10 characters. A portlet writes Hello, , which is 7 characters, and then portal!, 7 more.

On the second write, beforeWrite sees a count of 14. It swaps the branch for the null writer before portal! reaches the branch, then calls the callback, which empties the 7 characters already copied. The response gets all 14 characters. The cache gets none, and getCachedPortletData returns null.

The second write, after the limit trips

Nothing on that path uses a value coming back from the callback. The three versions of the call site differ only in how they deal with a value nobody wants:

The same reasoning explains the one Function the PR keeps. The PR also edits PortalRawEventsAggregatorImplTest, which drops a Function.apply result of its own. That function is the per-event callback that IPortalEventDao.aggregatePortalEvents takes, and its Boolean result tells the event data-access object (DAO) whether to keep going. That result means something, so the type keeps returning one, and the test assigns the value it drops to a local named ignored instead.

Code walkthrough

The path, end to end: getPrintWriter builds a LimitingTeeWriter with a lambda, each write runs beforeWrite, a write past the limit calls limitReachedCallback.accept(this), clearCachedWriter() empties the copy, and getCachedPortletData returns null.

1. The handler passes a lambda

getPrintWriter builds the writer tee with the handler's maximumSize, the real writer as out, the in-memory cachingWriter as the branch, and the callback last.

uPortal-rendering/src/main/java/org/apereo/portal/portlet/container/cache/CachingPortletOutputHandler.java:99-104

this.teeWriter =
        new LimitingTeeWriter(
                this.maximumSize,
                delegateWriter,
                this.cachingWriter,
                input -> clearCachedWriter());

Before the change, the last argument was an anonymous class. This is the removed code, quoted from the diff:

CachingPortletOutputHandler.java, removed lines

new Function<LimitingTeeWriter, Object>() {
    @Override
    public Object apply(LimitingTeeWriter input) {
        // Limit hit, clear the cache
        clearCachedWriter();
        return null;
    }
});

getOutputStream makes the same swap for the byte tee, with input -> clearCachedStream() at CachingPortletOutputHandler.java:130. The handler's import of com.google.common.base.Function goes, since nothing in the file uses it any more. The lambda ignores its argument, the tee that hit the limit, because the handler already holds its own reference to the copy it has to empty.

2. The tee stores a Consumer

LimitingTeeWriter keeps the limit in maximumCharacters, the original branch in branch, the callback, a running count in characterCount, and a flag, limitReached, for whether the limit has been reached.

uPortal-rendering/src/main/java/org/apereo/portal/portlet/container/cache/LimitingTeeWriter.java:29-34

public class LimitingTeeWriter extends TeeWriter {
    private final long maximumCharacters;
    private final Writer branch;
    private final Consumer<LimitingTeeWriter> limitReachedCallback;
    private long characterCount = 0;
    private boolean limitReached = false;

The field was private final Function<LimitingTeeWriter, ?> limitReachedCallback;. The matching parameter of the four-argument constructor changes the same way (LimitingTeeWriter.java:44), the import becomes java.util.function.Consumer (LimitingTeeWriter.java:19), and the class Javadoc now names a callback {@link Consumer} (LimitingTeeWriter.java:27). LimitingTeeOutputStream gets the identical edits, with its field at LimitingTeeOutputStream.java:32.

3. The tee fires it once

beforeWrite adds each write to the count, then checks the limit.

uPortal-rendering/src/main/java/org/apereo/portal/portlet/container/cache/LimitingTeeWriter.java:75-88

protected void beforeWrite(int n) throws IOException {
    this.characterCount += n;

    if (this.maximumCharacters > 0
            && !this.limitReached
            && this.characterCount > this.maximumCharacters) {
        // Hit limit, replace tee'd writer with a null writer
        this.limitReached = true;
        this.setBranch(NullWriter.NULL_WRITER);

        if (this.limitReachedCallback != null) {
            this.limitReachedCallback.accept(this);
        }
    }

!this.limitReached keeps the block from running a second time. The callback runs after the branch swap, so by the time the handler empties the copy, nothing more can land in it. These are the lines PR 2945 removes from the same spot, quoted from the diff:

LimitingTeeWriter.java, removed lines

// Named 'unused' to satisfy ErrorProne's CheckReturnValue; the callback's
// signature is Function<T,?> but its return value is intentionally discarded.
@SuppressWarnings("unused")
Object unused = this.limitReachedCallback.apply(this);

LimitingTeeOutputStream counts bytes instead of characters and swaps in NullOutputStream.NULL_OUTPUT_STREAM. Its call is at LimitingTeeOutputStream.java:84.

4. What the callback does, and what the handler returns

The lambda from step 1 calls clearCachedWriter, which deletes everything the copy holds. PR 2945 does not change it.

uPortal-rendering/src/main/java/org/apereo/portal/portlet/container/cache/CachingPortletOutputHandler.java:179-186

private void clearCachedWriter() {
    if (this.cachingWriter != null) {
        final StringBuilder builder = this.cachingWriter.getBuilder();
        if (builder.length() > 0) {
            builder.delete(0, builder.length());
        }
    }
}

When the portal later asks for the captured output, the handler checks the tee's flag first.

uPortal-rendering/src/main/java/org/apereo/portal/portlet/container/cache/CachingPortletOutputHandler.java:50-53

if ((this.teeWriter != null && this.teeWriter.isLimitReached())
        || (this.teeStream != null && this.teeStream.isLimitReached())) {
    // Hit the caching limit, nothing to return
    return null;

So the callback and the flag split the work. The flag keeps an oversized copy from being cached. The callback empties the partial copy as soon as the limit trips, while the portlet is still writing.

5. The tests switch to lambdas

Each tee's test class has a testContentExceedsClearBuffer case that passes a callback, sets the limit one below the content's length, and checks that the branch is empty after the limit trips. The writer test's anonymous class becomes a block lambda.

uPortal-webapp/src/test/java/org/apereo/portal/portlet/container/cache/LimitingTeeWriterTest.java:59-65

content.length() - 1,
NullWriter.NULL_WRITER,
stringWriter,
input -> {
    final StringBuilder builder = stringWriter.getBuilder();
    builder.delete(0, builder.length());
});

The stream test does the same with input -> byteStream.reset() (LimitingTeeOutputStreamTest.java:68), and both files drop their Guava Function import.

6. The one Function that stays

PortalRawEventsAggregatorImplTest uses Mockito to stand in for the event DAO. The stand-in's answer takes the per-event callback from the call's arguments and runs it on a mock event. That callback is the last parameter of IPortalEventDao.aggregatePortalEvents, and its type is Guava's Function, imported at IPortalEventDao.java:17.

uPortal-events/src/main/java/org/apereo/portal/events/handlers/db/IPortalEventDao.java:73-77

boolean aggregatePortalEvents(
        DateTime startTime,
        DateTime endTime,
        int maxEvents,
        Function<PortalEvent, Boolean> handler);

The Javadoc above it (IPortalEventDao.java:62-64) says the callback returns false to stop aggregation. The result carries a decision, so Consumer would throw that decision away. Before the change, the test called apply and dropped the result. These are the removed lines, quoted from the diff:

PortalRawEventsAggregatorImplTest.java, removed lines

((Function<PortalEvent, Boolean>) invocation.getArguments()[3])
        .apply(
                new MockPortalEvent(
                        this,
                        "serverName",
                        "eventSessionId",
                        person));

The test keeps the type and assigns the result to a local instead.

uPortal-webapp/src/test/java/org/apereo/portal/events/aggr/PortalRawEventsAggregatorImplTest.java:178-186

Boolean ignored =
        ((Function<PortalEvent, Boolean>)
                        invocation.getArguments()[3])
                .apply(
                        new MockPortalEvent(
                                this,
                                "serverName",
                                "eventSessionId",
                                person));

Quiz