[GAIAPLAT-1608] Fix gaia_ptr_t initialization in gaia_ptr_t::auto_connect_to_parent() #1043

simone-gaia · 2021-10-27T17:22:27Z

gaia_ptr_t::auto_connect_to_parent() was initializing gaia_ptr_t by passing the gaia_id to the constructor. The constructor though expects a locator. This lead to sporadic failures.

Remove auto in persistent_store_manager.cpp where the type cannot be easily inferred by the rhs.
Minor improvements to the direct_access example.
Fix warning in event_manager.cpp

…nect_to_parent()

senderista · 2021-10-27T18:22:31Z

Yikes, another reason to make locators/offsets/IDs strongly typed!

senderista

Given that this sort of bug is likely to recur, what do you think of removing all public constructors for gaia_ptr_t and replacing them with explicit factory methods like from_locator()?

production/examples/direct_access/hospital.cpp

LaurentiuCristofor · 2021-10-27T18:41:05Z

production/db/core/src/gaia_ptr_client.cpp

@@ -562,7 +562,7 @@ void gaia_ptr_t::auto_connect_to_parent(
            // record, we need to disconnect them.
            if (!has_connected)
            {
-                gaia_ptr_t child_ptr(child_id);
+                gaia_ptr_t child_ptr = gaia_ptr_t::open(child_id);


Weren't these lines equivalent? I would have expected them to be.

No, they aren't:

gaia_ptr_t::gaia_ptr_t(locator) gaia_ptr_t::open(gaia_id)

If there is a difference, we should try to eliminate it, otherwise this is a weakness of the API and will lead to further such issues.

I agree. @senderista had a suggestion. I would crate a follow-up jira for it.

Perhaps we should disable constructor initialization and have explicit open methods only: open_locator() and open_gaia_id(). Then at least the intention is clear and one could check more easily whether the expected parameter is passed in.

Feel free to do this in a followup PR if it's too much of a change for now, but do add a JIRA iterm if you plan to address it this way.

https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1635

LaurentiuCristofor · 2021-10-27T18:42:16Z

production/db/core/src/persistent_store_manager.cpp

@@ -121,10 +119,10 @@ void persistent_store_manager::prepare_wal_for_write(gaia::db::txn_log_t* log, c
    // The key_count variable represents the number of puts + deletes.
    size_t key_count = 0;
    // Obtain RocksDB transaction object.
-    auto txn = m_rdb_internal->get_txn_by_name(txn_name);
+    rocksdb::Transaction* txn = m_rdb_internal->get_txn_by_name(txn_name);


Thanks for fixing these. I don't like auto when I can't tell what's the underlying type.

simone-gaia · 2021-10-27T18:43:27Z

Given that this sort of bug is likely to recur, what do you think of removing all public constructors for gaia_ptr_t and replacing them with explicit factory methods like from_locator()?

I agree.

chuan · 2021-10-27T18:46:59Z

production/db/core/src/gaia_ptr_client.cpp

@@ -562,7 +562,7 @@ void gaia_ptr_t::auto_connect_to_parent(
            // record, we need to disconnect them.
            if (!has_connected)
            {
-                gaia_ptr_t child_ptr(child_id);
+                gaia_ptr_t child_ptr = gaia_ptr_t::open(child_id);


This is really strange. Do we use to have a constructor that takes gaia_id_t similar to what open does here? I am wondering otherwise how this can work in the first place. We do have a unit test (auto_connect_test.child_update_disconnect) for this scenario.

Do we use to have a constructor that takes gaia_id_t similar to what open does here?

Not sure about it. Created a follow up task here: https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1635

Agree, I do not understand how this worked in the first place.

OK. Seems like a regression from: d764f7c
I am still not sure why the unit test can pass though... 🤔

Do we use to have a constructor that takes gaia_id_t similar to what open does here?

Maybe, but we can only have a single constructor if the underlying type is the same integer type. This is why I was proposing using struct-based types instead of typedef-based types. See #801

I guess that because both gaia_id_t and gaia_locator_t are allocated sequentially in the current implementation, in some simple tests the 2 sequences advanced in lockstep and locator/gaia_id happened to coincide for the objects being tested.

There are 2 takeaways here, I think, one short-term and one long-term: in the short term we need to audit for and remove all public constructors that take types that are typedef-equivalent (like gaia_id_t/gaia_locator_t), and in the long term we need to make these types strongly compile-typed (like enum class). The latter goal has been discussed for a while but seems to have some difficult edge cases (at least one involving std::atomic IIRC). Here are 2 examples of this approach:

https://github.com/foonathan/type_safe
https://github.com/rollbear/strong_type

Ok. I attached the debugger. The locator and id in the unit test are indeed the same. Both were 84 in my debugging session.

It's tempting to randomize gaia_ids to flush out these bugs, but we can't really do that right now because we allow up to 2^32 locators, and if we generate 2^32 64-bit gaia_ids then we have ~50% probability of at least one collision.

LaurentiuCristofor

Left one suggestion for further improvement, to do now or later.

simone-gaia added 2 commits October 27, 2021 13:16

[GAIAPLAT-1608] Fix wrong gaia_ptr_t handling in gaia_ptr_t::auto_con…

f35f977

…nect_to_parent()

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform

dc9eaa4

simone-gaia requested review from chuan, senderista, LaurentiuCristofor and mihirj1993 October 27, 2021 17:22

simone-gaia changed the title ~~[GAIAPLAT-1608] Fix wrong gaia_ptr_t handling in gaia_ptr_t::auto_connect_to_parent()~~ [GAIAPLAT-1608] Fix gaia_ptr_t initialization in gaia_ptr_t::auto_connect_to_parent() Oct 27, 2021

senderista approved these changes Oct 27, 2021

View reviewed changes

production/examples/direct_access/hospital.cpp Outdated Show resolved Hide resolved

Address PR feedback

7c95962

simone-gaia commented Oct 27, 2021

View reviewed changes

production/examples/direct_access/hospital.cpp Outdated Show resolved Hide resolved

LaurentiuCristofor reviewed Oct 27, 2021

View reviewed changes

chuan reviewed Oct 27, 2021

View reviewed changes

LaurentiuCristofor approved these changes Oct 27, 2021

View reviewed changes

simone-gaia merged commit 1cb580c into master Oct 27, 2021

simone-gaia deleted the rondelli-fix-autoconnect-to-parent branch October 27, 2021 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GAIAPLAT-1608] Fix gaia_ptr_t initialization in gaia_ptr_t::auto_connect_to_parent() #1043

[GAIAPLAT-1608] Fix gaia_ptr_t initialization in gaia_ptr_t::auto_connect_to_parent() #1043

simone-gaia commented Oct 27, 2021

senderista commented Oct 27, 2021

senderista left a comment

LaurentiuCristofor Oct 27, 2021

simone-gaia Oct 27, 2021

LaurentiuCristofor Oct 27, 2021

simone-gaia Oct 27, 2021

LaurentiuCristofor Oct 27, 2021

simone-gaia Oct 27, 2021

LaurentiuCristofor Oct 27, 2021

simone-gaia commented Oct 27, 2021

chuan Oct 27, 2021

simone-gaia Oct 27, 2021

chuan Oct 27, 2021

LaurentiuCristofor Oct 27, 2021

senderista Oct 27, 2021

chuan Oct 27, 2021

senderista Oct 27, 2021

LaurentiuCristofor left a comment

[GAIAPLAT-1608] Fix gaia_ptr_t initialization in gaia_ptr_t::auto_connect_to_parent() #1043

[GAIAPLAT-1608] Fix gaia_ptr_t initialization in gaia_ptr_t::auto_connect_to_parent() #1043

Conversation

simone-gaia commented Oct 27, 2021

senderista commented Oct 27, 2021

senderista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-gaia commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuCristofor left a comment

Choose a reason for hiding this comment