Discovered a (probably race condition) bug with ChartIndicatorDelete.

New comment
 

Calling "Comment" and "ChartRedraw" directly before "ChartIndicatorDelete" causes it to fail with Error Code 4022 (ERR_PROGRAM_STOPPED)

After countless hours of work and hunting I finally managed to pinpoint the problem and come up with a precise minimalist code demonstration of the bug.

// !!! (Their calling order doesn't seem to matter.)
ChartRedraw(); Comment("abc"); // Remove either one of these calls to prevent the bug.
AssertError(!ChartIndicatorDelete(0, window, name))
// !!!

"AssertError" here is simply a macro that checks if the function returned false (which means it failed) and automatically alerts the last error. (It doesn't require a semicolon).

The following error will always occur: Error Code 4022: "Test forcibly stopped from the outside. For example, optimization interrupted, visual testing window closed or testing agent stopped"

The build that I ran this on is 5179. The error occurs regardless of compiler optimizations.

Interestingly enough, trying to run the code in debug mode by stepping and placing a breakpoint at ChartRedraw() seems to completely prevent the error. This makes it seem to me like it might be a race condition inside the MQL5 API.

Here is the full EA code that demonstrates this behavior.

#define AssertWith(expression, code) \
        if (expression) \
        { \
                Alert(#expression " has failed."); \
                code \
        }
        
#define Assert(expression) AssertWith(expression, return true;)

#define AssertError(expression) AssertWith(expression, AlertLastError(); return true;)

inline void AlertLastError() {Alert(GetErrorString(GetLastError()));}
inline string GetErrorString(int code) 
{return StringFormat("Error Code %d: \"%s\"", code, GetErrorDescription(code)); }
#include "GetErrorDescription.mqh"

int OnInit()
{
        AssertWith(Init(), return INIT_FAILED;)
        return INIT_SUCCEEDED;
}

void OnDeinit(const int reason)
{
        AssertWith(DeInit(), return;)
}

bool Init()
{
        Assert(AddIndicator())
        return false;
}

bool DeInit()
{
        Assert(RemoveIndicator())
        return false;
}

int window, handle;

bool AddIndicator()
{
        long total; AssertError(!ChartGetInteger(0, CHART_WINDOWS_TOTAL, 0, total))
        AssertError((handle = iCCI(Symbol(), Period(), 13, PRICE_TYPICAL)) == INVALID_HANDLE)
        AssertError(!ChartIndicatorAdd(0, window = (int)total++, handle))
        return false;
}

bool RemoveIndicator()
{
        ResetLastError();
        const string name = ChartIndicatorName(0, window, 0);
        PrintFormat("name = \"%s\"", name);
        AssertError(GetLastError())
        
        // !!! (Their calling order doesn't seem to matter.)
        ChartRedraw(); Comment("abc"); // Remove either one of these calls to prevent the bug.
        AssertError(!ChartIndicatorDelete(0, window, name))
        // !!!
        
        return false;
}
GetErrorDescription is obviously simply a long function that associates each error code with a string description. Would've been too long to include it in the same file. You can replace it with a dummy function on your end.
 
I've thought about it a while and I've acknowledged the possibility that maybe I'm the stupid one here. Throughout the docs it is mentioned that some functions such as CopyBuffer act asynchronously and simply fail (return -1) if the resource is not ready. If my understanding is correct, if the programmer wants it to behave like a synchronous call then he has to write a loop with a bunch of Sleep(...) calls.

The thing is, though, none of this seems to be mentioned in the documentation about graphical objects and charts. Of course there are mentions of some function calls being added on some sort of queue, but nowhere is it mentioned that functions like ChartIndicatorDelete would benefit from a synchronous adaptation.
New comment