...
 
Commits (6)
  • Aleksy Barcz's avatar
    Removed signal handlers from draw3 · 2275b41d
    Aleksy Barcz authored
    + caused deadlocks on program exit, as functions called from signal handler were not async-signal-safe (see: man signal-safety). Amongst others, malloc and pthread_join are not safe.
    + OS will cleanup our threads and db queue anyway
    + saving defined user params on exit is no longer necessary as we save them on every edit action
    + because of all the above, we can safely leave signal handling and cleanup to standard OS behavior on Linux
    2275b41d
  • Aleksy Barcz's avatar
    libparnt: lock everything · 9ce9aa92
    Aleksy Barcz authored
    + there is a lock introduced in "Add global parser synchronous access mutex", but still draw3 segfaults on init
    + segfaults are not reproducible using valgrind, and happen in AddPar, where curr points to a badly initialized structure
    + altogether this looks very much like a multithreading issue
    + so introduce a recursive_lock, locking every single function in libparnt
    + this is extremely ugly, but it works and it doesn't slow down program start remarkably
    + a proper fix would be removing all the libparnt code and writing a new, simple, parser for szarp.cfg
    9ce9aa92
  • Michał Glinka's avatar
    Merge branch 'rc' into devel · e471e6f7
    Michał Glinka authored
    e471e6f7
  • Aleksy Barcz's avatar
    deadlock in UDIC/UDIM fix continuation · ec8d2a29
    Aleksy Barcz authored
    + follow-up of "fixed deadlock in defined initialization"
    + previous fix fixed the deadlock in one case, but not in the general case, as unfortunately there are other call-traces that lead to the deadlock
    + this fixes a sequence, where thread 1 calls UDIM::AddUserDef (acquires mutex), which calls UDIC::AddUserDef (acquires m_lock); while thread 2 calls PCIC::GetConfig (acquires m_lock), which calls UDIC::AddConfig, which calls UDIM::PopConfig (acquires mutex)
    + the general design flaw is that UDIM can call UDIC and vice versa. This is wrong and especially bug-prone when using locks.
    + this fix should fix the general case. The design flaw remains to be fixed, but now whenever UDIM calls UDIC, it does so without holding mutex (fixed both places in code where I found such a situation). So both locks are held only on the path from UDIC to UDIM and not vice versa.
    ec8d2a29
  • Aleksy Barcz's avatar
    launch query executor after all configs are loaded · 3f3418cc
    Aleksy Barcz authored
    + launch query executor as the last step of OnInit
    + this fixes a segfault when QueryExecutor tries to SetProberAddress while OnInit didn't finish loading configurations
    + probably this worked previously only because configurations loaded fast enough
    3f3418cc
  • Aleksy Barcz's avatar
    workaround for unitialized memory read on import set · 936b3617
    Aleksy Barcz authored
    + if we import a set in draw3 (from .xsd file), defcfg won't set _configId in imported params, leaving it uninitialized and leading to strange errors
    + initialize _configId to invalid value, so at worst it will fail in a more comprehensible way
    + we cannot throw from GetConfigId because of the "Caught unhandled unknown exception; terminating" wx behaviour which hides the place in code from which an exception is thrown. Overriding OnUnhandledException and OnExceptionInMainLoop in szapp (and e.g. calling abort, terminate from there) has no effect.
    + introduced a workaround in a completely different part of code than importing set, as the bug in importing set would be hard to find
    936b3617
......@@ -45,6 +45,8 @@
Par *globals = NULL;
Sections *sect = NULL;
static std::recursive_mutex libparnt_mutex;
namespace {
bool libpar_initted = false;
};
......@@ -61,6 +63,7 @@ void libpar_reset();
Sections *AddSection(const char *name)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Sections *nowa, *curr;
if (strcasecmp(name, "global") == 0)
......@@ -94,6 +97,7 @@ Sections *AddSection(const char *name)
void AddPar(Par ** list, const char *name, const char *content)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Par *nowa, *curr;
/* replace existing parameter content if param already exists */
......@@ -135,6 +139,7 @@ void AddPar(Par ** list, const char *name, const char *content)
void DeleteParList(Par ** list)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Par *curr, *nextcurr;
if (!(*list))
......@@ -152,6 +157,7 @@ void DeleteParList(Par ** list)
void DeleteSectList()
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Sections *curr, *nextcurr;
if (!sect)
......@@ -169,6 +175,7 @@ void DeleteSectList()
char *SeekPar(Par * list, const char *name)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Par *curr;
int found;
......@@ -191,6 +198,7 @@ char *SeekPar(Par * list, const char *name)
Sections *SeekSect(const char *name)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Sections *curr;
int found;
......@@ -215,12 +223,14 @@ Sections *SeekSect(const char *name)
#ifndef MINGW32
void libpar_init()
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
libpar_init_with_filename(NULL, 1);
}
//load 'szarp.cfg' from folder. Path to a directory must end with '/'
void libpar_init_from_folder(std::string folder_path)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
if(folder_path.empty())
return; //empty string
std::string config_name = CFGNAME;
......@@ -246,6 +256,7 @@ void libpar_init_from_folder(std::string folder_path)
*/
void libpar_read_cmdline(int *argc, char *argv[])
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
int i, j;
int l;
char *endptr;
......@@ -320,6 +331,7 @@ void libpar_read_cmdline(int *argc, char *argv[])
*/
void libpar_read_cmdline_w(int *argc, wchar_t *argv[])
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
int i, j;
int l;
wchar_t *endptr;
......@@ -386,6 +398,7 @@ void libpar_read_cmdline_w(int *argc, wchar_t *argv[])
int libpar_init_with_filename(const char *filename, int exit_on_error)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
if( libpar_initted )
libpar_reset();
libpar_initted = true;
......@@ -433,6 +446,7 @@ int libpar_init_with_filename(const char *filename, int exit_on_error)
void libpar_done()
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
DeleteParList(&globals);
DeleteParList(&command_line_pars);
DeleteSectList();
......@@ -441,6 +455,7 @@ void libpar_done()
void libpar_getkey(const char *section, const char *par, char **buf)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
Sections *s;
char *c;
......@@ -460,6 +475,7 @@ void libpar_getkey(const char *section, const char *par, char **buf)
void libpar_readpar(const char *section, const char *par, char *buf,
int size, int exit_on_error)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
char *c;
libpar_getkey(section, par, &c);
......@@ -477,6 +493,7 @@ void libpar_readpar(const char *section, const char *par, char *buf,
char *libpar_getpar(const char *section, const char *par, int exit_on_error)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
char *c;
libpar_getkey(section, par, &c);
......@@ -492,6 +509,7 @@ char *libpar_getpar(const char *section, const char *par, int exit_on_error)
}
void libpar_reset() {
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
libpar_initted = false;
int args_count = 0;
int i;
......@@ -543,23 +561,27 @@ void libpar_reset() {
#ifndef MINGW32
void libpar_reinit() {
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
libpar_reset();
libpar_init();
}
#endif
void libpar_hard_reset() {
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
libpar_reset();
parser_destroy();
}
void libpar_reinit_with_filename(const char *name, int exit_on_error) {
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
libpar_reset();
libpar_init_with_filename(name, exit_on_error);
}
void libpar_reinit_from_folder(const std::string& folder_path)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
libpar_reset();
libpar_init_from_folder(folder_path);
}
......@@ -568,6 +590,7 @@ void libpar_reinit_from_folder(const std::string& folder_path)
void libpar_setXenvironment(const char *programname)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
#define XENV "XENVIRONMENT"
char *c;
static char buf[1000];
......@@ -585,6 +608,7 @@ void libpar_setXenvironment(const char *programname)
void libpar_printfile(char *programname, char *printcmd, char *filename)
{
std::lock_guard<std::recursive_mutex> lguard(libparnt_mutex);
char *c, *ss;
libpar_getkey(programname, printcmd, &c);
......
......@@ -840,7 +840,7 @@ public:
void SetParamId(unsigned paramId) { _paramId = paramId; }
unsigned GetConfigId() const { return _configId; }
int GetConfigId();
void SetConfigId(unsigned configId) { _configId = configId; }
......@@ -868,7 +868,7 @@ protected:
unsigned _paramId;
unsigned _configId;
int _configId{-1}; // if left uninitialized, this can lead to ugly errors, so initialize to invalid value
TRaport * _raports; /**< List of raports for param */
TDraw * _draws; /**< List of draws for param */
......
......@@ -750,6 +750,17 @@ std::wstring TParam::GetGlobalName() const {
return sc->GetPrefix() + L":" + _name;
}
int TParam::GetConfigId()
{
if (_configId < 0) {
// _configId should be set, but it's not so let's try to workaround it
if (GetSzarpConfig()) {
SetConfigId(GetSzarpConfig()->GetConfigId());
}
}
return _configId;
}
namespace szarp {
std::wstring substituteWildcards(const std::wstring &name, const std::wstring &ref)
......
......@@ -58,15 +58,15 @@ TSzarpConfig* UserDefinedIPKManager::PopConfig(const std::wstring &prefix) {
}
void UserDefinedIPKManager::AddUserDefined(const std::wstring& prefix, TParam *param) {
boost::unique_lock<boost::shared_mutex> lock(mutex);
if (ipk->AddUserDefined(prefix, param)) {
boost::unique_lock<boost::shared_mutex> lock(mutex);
m_extra_params[prefix].emplace_back(param);
}
}
void UserDefinedIPKManager::RemoveUserDefined(const std::wstring& prefix, TParam *param) {
boost::unique_lock<boost::shared_mutex> lock(mutex);
if (ipk->RemoveUserDefined(prefix, param)) {
boost::unique_lock<boost::shared_mutex> lock(mutex);
auto& tp = m_extra_params[prefix];
auto ei = std::find_if(tp.begin(), tp.end(),
[param] (std::shared_ptr<TParam> &_p) { return _p.get() == param; }
......
......@@ -151,12 +151,6 @@ extern void InitXmlResource();
bool read_only;
std::unique_ptr<FileLocker> m_instance;
void handler(int sig)
{
wxGetApp().OnExit();
_exit(0);
}
namespace {
int GL_ATTRIBUTES[] = {
WX_GL_RGBA,
......@@ -241,9 +235,6 @@ bool DrawApp::OnInit() {
SetProgName(_T("Draw 3"));
signal(SIGHUP, handler);
signal(SIGINT, handler);
signal(SIGTERM, handler);
if (m_just_print_version) {
std::cout << SZARP_VERSION << std::endl;
......@@ -424,12 +415,6 @@ bool DrawApp::OnInit() {
m_dbmgr->SetBaseHandler(base_handler);
m_dbmgr->SetProbersAddresses(GetProbersAddresses());
splash->PushStatusText(_("Starting database query mechanism..."));
m_executor = new QueryExecutor(m_db_queue, m_dbmgr, m_dbmgr->GetBaseHandler());
m_executor->Create();
m_executor->SetPriority((WXTHREAD_MAX_PRIORITY + WXTHREAD_DEFAULT_PRIORITY) / 2);
m_executor->Run();
m_cfg_mgr->SetDatabaseManager(m_dbmgr);
......@@ -493,6 +478,12 @@ bool DrawApp::OnInit() {
return FALSE;
}
splash->PushStatusText(_("Starting database query mechanism..."));
m_executor = new QueryExecutor(m_db_queue, m_dbmgr, m_dbmgr->GetBaseHandler());
m_executor->Create();
m_executor->SetPriority((WXTHREAD_MAX_PRIORITY + WXTHREAD_DEFAULT_PRIORITY) / 2);
m_executor->Run();
wxToolTip::SetDelay(1000);
SetAppName(_T("SZARPDRAW3"));
......
......@@ -96,7 +96,7 @@ class DrawApp : public DrawGLApp
{
public:
/**
* Method called on application end and on sigint.
* Method called on application end.
*/
int OnExit();
......
......@@ -306,7 +306,7 @@ void RemarksHandler::GetConfigurationFromSzarpCfg() {
wxLogWarning(_T("defined __WXGTK__"));
//init libpar from folder, default: /opt/szarp/<base-name>
libpar_init_from_folder(std::string(m_config_manager->GetSzarpDir().mb_str()));
libpar_reinit_from_folder(std::string(m_config_manager->GetSzarpDir().mb_str()));
/* Check for remarks_server option in config file */
char *server = libpar_getpar("", "remarks_server", 0);
......