sysrepo::Lock destructor not respecting the datastore the lock was created for

[rbu9fe]

The following code snipped leads to an error saying that a Yang module was not locked by this session:

{
  session.switchDatastore( sysrepo::Datastore::Candidate );
  sysrepo::Lock lock( session );
  session.switchDatastore( sysrepo::Datastore::Running );
}

Looks like the Lock destructor tries to call sr_unlock for the wrong datastore. Even after session/process termination the candidate datastore remains locked in shared memory by the destroyed session.

[jktjkt]

@michalvasko, this report says that both sr_lock() and sr_unlock() operate on the "currently chosen datastore", not on the original one that was active in that session at the time of the lock. Can you please advice whether this is expected behavior of sysrepo?

[michalvasko]

Yes, this is expected, all the functions that use the datastore of the session use the current one, it is not stored anywhere.

[rbu9fe]

Since sysrepo is behaving as expected imo sysrepo-cpp should call sr_unlock after temporarily switching to the datastore that was active when the lock was acquired in order to support the RAII principle.

[rbu9fe]

Is there any progress on this? Wouldn't it make sense to track the active datastore when constructing Lock (e.g. in m_locked_datastore) and change the destructor to something like the following?

sr_datastore_t curr_datastore = sr_session_get_ds(session);
sr_session_switch_ds(session, m_locked_datastore);
sr_unlock(session, module, timeout);
sr_session_switch_ds(session, curr_datastore);

[jktjkt]

I was afraid that this might be tricky to do safely with RAII because these functions could fail, which would mean an exception, except that when that happens and another exception is already being handled, boom, I mean, std::terminate. But these apparently won't fail, so, https://gerrit.cesnet.cz/c/CzechLight/sysrepo-cpp/+/6831 .

[rbu9fe]

Great, thanks a lot!