Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes recursive semaphore lock timeout [ch21928] #1577

Merged
merged 3 commits into from Sep 28, 2018

Conversation

@technobly
Copy link
Member

commented Sep 28, 2018

Problem

If the system thread or application thread gets stuck attempting to dynamically allocation memory, and the opposing thread subsequently also attempts to allocate memory, the recursive semaphore will deadlock. This recursive semaphore does have a timeout, but was set to approximately 1193 hours, so basically it would be stalled forever.

Solution

  • The semaphore lock timeout was changed to 60 seconds, and a new PANIC SOS 14 "Semaphore Lock Timeout" was added to note this condition and reset the device. The PANIC code is also saved in the last Reset Reason.
  • Additionally SPARK_ASSERT() was added to ensure __malloc_lock/unlock is not called from an ISR.

Steps to Test

This semaphore deadlock condition is so rare, I could not create a simple user app to force it to occur. The new PANIC was tested just by adding the panic to a user app, essentially forcing the panic code.

FWIW, Here's my test app (that doesn't cause the issue)

#include "Particle.h"

SYSTEM_THREAD(ENABLED);

SerialLogHandler logHandler(115200, LOG_LEVEL_ALL);

volatile bool run = true;

Thread *genericAllocatorThread;
Thread *stringAllocatorThread;
Thread *memoryLeakThread;
Thread *recursiveMallocThread;

void createAStringFromThread() {
    String string1 = String("A fun") + String("new string") + String("from thread! OMG omg omg omg let's make it longer!!!!!");
    // Serial.println(string1.c_str());
}

void createAStringFromApp() {
    String string2 = String("A fun") + String("new string") + String("from app! OMG omg omg omg let's make it longer!!!!!");
    // Serial.println(string2.c_str());
}

void createAStringRecursively() {
    String string2 = String("A fun") + String("new string") + String("recursively! OMG omg omg omg let's make it longer!!!!!");
    delay(5000);

    // forever!!
    createAStringRecursively();
}

void semaphoreLockTest() {

    // Call generic malloc in one thread
    genericAllocatorThread = new Thread("generic-allocator", [&]
    {
        while(run)
        {
            void *ptr = malloc(100);
            free(ptr);
        }
    });

    // Call String use of malloc in another thread
    stringAllocatorThread = new Thread("string-allocator", [&]
    {
        while(run)
        {
            createAStringFromThread();
        }
    });

    // Memory leak in another thread
    memoryLeakThread = new Thread("memory-leak", [&]
    {
        while(run)
        {
            static uint32_t last = millis();
            if (millis() - last >= 500) {
                last = millis();
                String *ptr1 = new String("The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!!");
                String *ptr2 = new String("The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!! The faucet is on!!!");
                delete ptr1;
                // String *ptr3 = new String("The faucet is on!!!");
                // String *ptr4 = new String("The faucet is on!!!");
                // delete ptr4;
            }
        }
    });

    // Create a string recursively (although this isn't a true recursive malloc operation) ... basically it's another memory leak
    recursiveMallocThread = new Thread("recursive-malloc", [&]
    {
        while(run)
        {
            createAStringRecursively();
            while(1); // should never get here
        }
    });
}

bool once = false;
void setup() {
    pinMode(D7, OUTPUT);

    // Kick off the test before we are connected, this usually results in handshake errors or running out of memory before we get connected.
    // semaphoreLockTest();
    // once = true;
}

void loop() {
    static uint32_t last_memory_update = millis();
    static runtime_info_t info;
    if (info.size == 0) {
        info.size = sizeof(info);
    }

    // If not already running from setup(), enable after we connect to the cloud.
    if (!once && Particle.connected()) {
        once = true;
        semaphoreLockTest();
    }

    // Every second log the memory stats
    if (millis() - last_memory_update >= 1000UL) {
        last_memory_update = millis();
        digitalWrite(D7, !digitalRead(D7));
        HAL_Core_Runtime_Info(&info, nullptr);
        Serial.printlnf("\r\n\r\n||| mem:%lu, heap:%lu\r\n", info.freeheap, info.largest_free_block_heap);
    }

    // as fast as possible!
    if (run) {
        createAStringFromApp();
    }

    // End the test at 3 minutes
    if (millis() > 180000) {
        run = false;

        // No assertion needed. If the test finishes there was no deadlock
        genericAllocatorThread->dispose();
        stringAllocatorThread->dispose();
        memoryLeakThread->dispose();
        recursiveMallocThread->dispose();

        Serial.println("\r\n\r\n||| Test Passed!!!\r\n");

        // Optionally force this new Panic Code just as a test
        PANIC(SemaphoreLockTimeout,"Semaphore Lock Timeout");

        while(1) {
            Particle.process();
        }
    }

}

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation (Add new PANIC code)
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@technobly technobly added the bug label Sep 28, 2018

@technobly technobly added this to the 0.8.0-rc.11 milestone Sep 28, 2018

@technobly technobly requested a review from sergeuz Sep 28, 2018

@technobly technobly merged commit c12bd46 into develop Sep 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the fix/semaphore-lock branch Sep 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.