<html><head></head><body style="color: rgb(0, 0, 0); font-size: 14px; font-family: Calibri, sans-serif; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">Hi,</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">I got this assertion error from Patrik Wallström yesterday and been looking into it some:</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div><font class="Apple-style-span" face="Calibri,sans-serif">ods-signerd: ../../../signer/src/daemon/cmdhandler.c:322: cmdhandler_handle_cmd_sign: assertion zone->task failed</font></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">From what I can see the problem is that zonelist_update() may leave zone objects with a zone->task set to NULL before engine_update_zones() can assign a task to them. This can happen inside cmdhandler_handle_cmd_update() because it releases the lock on the zonelist before running engine_update_zones():</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">1. cmdhandler_handle_cmd_update : lock zonelist</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">2. cmdhandler_handle_cmd_update : zonelist_update()</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">3. cmdhandler_handle_cmd_update : unlock zonelist</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">4. cmdhandler_handle_cmd_update : engine_update_zones()</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">5. engine_update_zones : lock zonelist</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">6. engine_update_zones : update zones</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">7. engine_update_zones : unlock zonelist</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">Some other thread might try and lock the zonelist at step 2 and then it would get that lock before engine_update_zones() can do its part.</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">A quick fix to this is to make engine_update_zones() aware of the lock on zonelist so it doesn't try and lock it and release it after, suggested patch below.</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">Altho I am seeing this almost everywhere and I would like to discuss our approach. I would really like to see read and write locks implemented to better support thread interoperability. Using pthread_mutex_trylock()/pthread_mutex_timedlock() where it suites to counter hangs. Using PTHREAD_MUTEX_RECURSIVE so different segments can lock the same locked object without letting each other know providing better OO. Of course as complexity in the locking mechanism increases so is the chance of dead locks.</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">Why this hasn't turned up more might be that each zone is handled by one thread, only time two threads might be working on the same zone is if you do manually commands, and that the locking right now is very loose.</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; ">Thoughts?</div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><div>/Jerry</div></div></div></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><br></div><div style="color: rgb(0, 0, 0); font-family: Calibri, sans-serif; font-size: 14px; "><div>Index: signer/src/daemon/cmdhandler.c</div><div>===================================================================</div><div>--- signer/src/daemon/cmdhandler.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 5654)</div><div>+++ signer/src/daemon/cmdhandler.c<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -190,11 +190,11 @@</div><div> cmdc->engine->zonelist->just_removed = 0;</div><div> cmdc->engine->zonelist->just_added = 0;</div><div> cmdc->engine->zonelist->just_updated = 0;</div><div>- lock_basic_unlock(&cmdc->engine->zonelist->zl_lock);</div><div> </div><div> ods_log_debug("[%s] commit zone list changes", cmdh_str);</div><div>- engine_update_zones(cmdc->engine);</div><div>+ engine_update_zones(cmdc->engine, 1);</div><div> ods_log_debug("[%s] signer configurations updated", cmdh_str);</div><div>+ lock_basic_unlock(&cmdc->engine->zonelist->zl_lock);</div><div> } else {</div><div> lock_basic_unlock(&cmdc->engine->zonelist->zl_lock);</div><div> (void)snprintf(buf, ODS_SE_MAXLINE, "Zone list has errors.\n");</div><div>Index: signer/src/daemon/engine.c</div><div>===================================================================</div><div>--- signer/src/daemon/engine.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 5654)</div><div>+++ signer/src/daemon/engine.c<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -811,7 +811,7 @@</div><div> *</div><div> */</div><div> void</div><div>-engine_update_zones(engine_type* engine)</div><div>+engine_update_zones(engine_type* engine, int zonelist_is_locked)</div><div> {</div><div> ldns_rbnode_t* node = LDNS_RBTREE_NULL;</div><div> zone_type* zone = NULL;</div><div>@@ -833,8 +833,10 @@</div><div> now = time_now();</div><div> reload_zonefetcher(engine);</div><div> </div><div>- lock_basic_lock(&engine->zonelist->zl_lock);</div><div>- /* [LOCK] zonelist */</div><div>+ if (!zonelist_is_locked) {</div><div>+ <span class="Apple-tab-span" style="white-space:pre"> </span>lock_basic_lock(&engine->zonelist->zl_lock);</div><div>+ /* [LOCK] zonelist */</div><div>+ }</div><div> node = ldns_rbtree_first(engine->zonelist->zones);</div><div> while (node && node != LDNS_RBTREE_NULL) {</div><div> zone = (zone_type*) node->data;</div><div>@@ -929,8 +931,10 @@</div><div> }</div><div> node = ldns_rbtree_next(node);</div><div> }</div><div>- /* [UNLOCK] zonelist */</div><div>- lock_basic_unlock(&engine->zonelist->zl_lock);</div><div>+ if (!zonelist_is_locked) {</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>/* [UNLOCK] zonelist */</div><div>+<span class="Apple-tab-span" style="white-space:pre"> </span>lock_basic_unlock(&engine->zonelist->zl_lock);</div><div>+ }</div><div> if (wake_up) {</div><div> engine_wakeup_workers(engine);</div><div> }</div><div>@@ -1097,7 +1101,7 @@</div><div> /* update zones */</div><div> if (zl_changed == ODS_STATUS_OK) {</div><div> ods_log_debug("[%s] commit zone list changes", engine_str);</div><div>- engine_update_zones(engine);</div><div>+ engine_update_zones(engine, 0);</div><div> ods_log_debug("[%s] signer configurations updated", engine_str);</div><div> zl_changed = ODS_STATUS_UNCHANGED;</div><div> }</div><div>Index: signer/src/daemon/engine.h</div><div>===================================================================</div><div>--- signer/src/daemon/engine.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 5654)</div><div>+++ signer/src/daemon/engine.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -101,7 +101,7 @@</div><div> * \param[in] engine engine</div><div> *</div><div> */</div><div>-void engine_update_zones(engine_type* engine);</div><div>+void engine_update_zones(engine_type* engine, int zonelist_is_locked);</div><div> </div><div> /**</div><div> * Clean up engine.</div><div><br></div></div></body></html>