Memory leak between AudioPlayer/ProgressTimer

[baustinanki]

Briefly summarize your issue:

When shutting down audio playback while Alexa is telling me a story (following "Alexa, tell me a story about yourself"), the AudioPlayer instance is not deleted, due to the ProgressTimer it owns still having a shared_ptr reference to it.

What is the expected behavior?

AudioPlayer::doShutdown() should have the effect of removing shared references to itself that it's given to other modules, allowing its deletion.

What behavior are you observing?

AudioPlayer::m_progressTimer holds a shared reference to the AudioPlayer instance that prevents its deletion. The sequence is illustrated by the following log lines.

AudioPlayer receives a play directive in JSON, but it does not have a progressReport section:

[  f] 1 AudioPlayer:handlePlayDirective
[  f] E JsonUtils:findNodeFailed:reason=missingDirectChild,child=progressReport

Thus, the following code in AudioPlayer.cpp sets default interval/delay values:

    rapidjson::Value::ConstMemberIterator progressReport;
    audioItem.stream.progressReport.delay = ProgressTimer::NO_DELAY;
    audioItem.stream.progressReport.interval = ProgressTimer::NO_INTERVAL;
    if (!jsonUtils::findNode(stream->value, "progressReport", &progressReport)) {
        progressReport = stream->value.MemberEnd();
    } else {
        // the code here would set non-default values if they were present, but they aren't

When m_progressTimer.start() is later called upon audio playback beginning, it detects these default values of NO_DELAY/NO_INTERVAL and decides to set its internal state back to IDLE (this code is in ProgressTimer::start():

    if (m_delay != NO_DELAY) {
        if (m_interval != NO_INTERVAL) {
            m_target = std::min(m_delay, m_interval * ((m_offset / m_interval) + 1));
        } else {
            m_target = m_delay;
        }
    } else {
        if (m_interval != NO_INTERVAL) {
            m_target = m_interval * ((m_offset / m_interval) + 1);
        } else {
            ACSDK_DEBUG5(LX("startNotStartingThread").d("reason", "noTarget"));
--->        setState(State::IDLE);
            return;
        }
    }

The debug line there shows up in my log:

[  8] 5 ProgressTimer:startNotStartingThread:reason=noTarget

With the ProgressTimer state now set back to IDLE, the following happens in ProgressTimer::stop():

void ProgressTimer::stop() {
    ACSDK_DEBUG5(LX(__func__));

    // If we are already stopped, there is nothing to do.
    {
        std::lock_guard<std::mutex> stateLock(m_stateMutex);
        if (State::IDLE == m_state) {
--->        return;
        }
    }

This is bad, because it means the m_context shared pointer (back to the AudioPlayer that owns it) does not get reset at the end of the function:

    // ...below above code, from which we've already done an early return

    m_context.reset();
    m_delay = NO_DELAY;
    m_interval = NO_INTERVAL;
    m_target = std::chrono::milliseconds::zero();
}

Thus, the ProgressTimer maintains its shared reference back to the AudioPlayer, preventing its deletion (along with a couple other components the AudioPlayer holds shared references to, like the exception sender and message sender).

Provide the steps to reproduce the issue, if applicable:

1. Open sample app
2. Hit t, press [enter]
3. Say "tell me a story about yourself"
4. When Alexa replies, hit q then [enter]
5. Look at log output and observe:

2018-12-14 01:03:30.716 [0] W RequiresShutdown:ShutdownMonitor:reason=never deleted,name=AudioPlayer

This is evidence of the memory leak.

Tell us about your environment:

What version of the AVS Device SDK are you using?

1.10

Tell us what hardware you're using:

• Desktop / Laptop
• Raspberry Pi
• Other - tell us more:

Tell us about your OS (Type & version):

• Linux
• MacOS
• Raspbian Stretch
• Raspbian Jessy
• Other - tell us more:

[scotthea-amazon]

Hello baustinanki,

Thank you very much for tracking this down and providing such a clear description of the problem! Your analysis looks correct to us.

Best regards,
-SWH

[scotthea-amazon]

This is fixed in v1.11. Closing this issue. If you think this issue has been closed in error, please re-open it and include a comment describing why it should not have been closed.