GEODE-9753: Solve PDX serialization coredump #891

gaussianrecurrence · 2021-11-05T11:13:19Z

While serializing a PdxSerializable object, there is a possible race
condition which might cause the client to crash.
This race-condition happens whenever the cluster is restarted during
the serialization process and if
on-client-disconnect-clear-pdxType-Ids is set to true, meaning the
PdxTypeRegistry will be cleaned up if the connection towards the
cluster is lost.
This issue has been solved by using the previously fetched local PDX
type.
In order to properly test this solution, PdxRemoteWriterFactory has been added,
so the race-condition can be force at test-time.
A new IT has been added to test that the solution is working fine.
make_unique was needed inside cppcache/src, so it was moved to
utils/cxx_extensions.hpp

- While serializing a PdxSerializable object, there is a possible race condition which might cause the client to crash. This race-condition happens whenever the cluster is restarted during the serialization process and if on-client-disconnect-clear-pdxType-Ids is set to true, meaning the PdxTypeRegistry will be cleaned up if the connection towards the cluster is lost. - This issue has been solved by using the previously fetched local PDX type. - In order to properly test this solution, PdxRemoteWriterFactory has been added, so the race-condition can be force at test-time. - A new IT has been added to test that the solution is working fine. - make_unique was needed inside cppcache/src, so it was moved to utils/cxx_extensions.hpp

gaussianrecurrence · 2022-01-13T09:33:50Z

I am reviewing all of my pending PRs, and this has been awating review for quite some time. Maybe @pdxcodemonkey, @pivotal-jbarrett @albertogpz @mkevo @jvarenina or anyone else could take a look at it and provide some feedback?
Thanks!

pdxcodemonkey · 2022-02-22T22:23:16Z

cppcache/src/PdxHelper.cpp

-        return PdxRemoteWriter(output, className, pdxTypeRegistry);
-      }
-    };
+    auto prw = cacheImpl->getPdxRemoteWriterFactory().create(


Can we use a slightly longer, meaningful name? like remoteWriter for instance?

pdxcodemonkey · 2022-02-22T22:27:51Z

cppcache/src/PdxRemoteWriter.hpp

 namespace apache {
 namespace geode {
 namespace client {

-class PdxRemoteWriter : public PdxLocalWriter {
+class APACHE_GEODE_EXPORT PdxRemoteWriter : public PdxLocalWriter {


Please don't export this, we can't change the ABI and anyway we don't want any more publicly-accessible things than we already have(!).

This is exported for testing purposes. Will try to test it the way @pivotal-jbarrett, which would avoid exporting symbols.

pdxcodemonkey · 2022-02-22T22:31:45Z

cppcache/src/PdxRemoteWriterFactoryImpl.hpp

+namespace geode {
+namespace client {
+
+class APACHE_GEODE_EXPORT PdxRemoteWriterFactoryImpl : public virtual PdxRemoteWriterFactory {


Again, please don't export.

This is exported for testing purposes. Will try to test it the way @pivotal-jbarrett, which would avoid exporting symbols.

jake-at-work · 2022-02-22T23:25:29Z

cppcache/src/PdxHelper.cpp

-        return PdxRemoteWriter(output, className, pdxTypeRegistry);
-      }
-    };
+    auto prw = cacheImpl->getPdxRemoteWriterFactory().create(


What this extracted into an pure virtual interface for the sole purpose of integration testing?

Indeed. I've seen you proposed an alternative way of testing this. I'll look into it.

jake-at-work · 2022-02-22T23:28:14Z

This issue has been solved by using the previously fetched local PDX
type.

Do you mean this issue is solved in this PR or in a previous and this PR just adds more testing?

jake-at-work · 2022-02-22T23:30:51Z

cppcache/src/PdxHelper.cpp

-      cachePerfStats.incPdxSerialization(
-          pdxLen + 1 + 2 * 4);  // pdxLen + 93 DSID + len + typeID
-    }
+    uint8_t* stPos =


jake-at-work · 2022-02-22T23:31:29Z

cppcache/src/PdxHelper.cpp

-      cachePerfStats.incPdxSerialization(
-          pdxLen + 1 + 2 * 4);  // pdxLen + 93 DSID + len + typeID
-    }
+    uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +


cppcache/src/PdxHelper.cpp

jake-at-work · 2022-02-22T23:33:59Z

cppcache/src/PdxRemoteWriter.cpp

  }
-  initialize();
+
+  m_pdxClassName = pdxType->getPdxClassName();


If pdxType is nullable then this line will throw.

jake-at-work · 2022-02-22T23:34:33Z

cppcache/src/PdxRemoteWriter.cpp

      m_preserveDataIdx(0),
      m_currentDataIdx(-1),
      m_remoteTolocalMapLength(0) {
-  m_preserveData = nullptr;
  if (m_pdxType != nullptr) {


If pdxType is not nullable then this check is unnecessary.

Thing is I did leave that because I thought what if pdxType passed were nullptr, but that's not the way it's intended to be used, so I guess it would be best to force a coredump if pdxType is nullptr, right?

Yeah, if it is nullptr then line 63 is going to fault anyway. Also, if passed here as nulltpr but later use excepted m_pdxType to not be nullptr... boom.

jake-at-work · 2022-02-22T23:36:10Z

cppcache/src/PdxRemoteWriter.hpp

                  std::shared_ptr<PdxTypeRegistry> pdxTypeRegistry);

-  PdxRemoteWriter(DataOutput& output, std::string pdxClassName,
+  PdxRemoteWriter(DataOutput& output, std::shared_ptr<PdxType> pdxType,
+                  std::shared_ptr<PdxRemotePreservedData> preservedData,


It this really feels like an extension of the other constructor with the optional preservedData parameters to maybe re-order the parameters to look like that.

jake-at-work · 2022-02-22T23:40:36Z

cppcache/src/PdxRemoteWriterFactoryImpl.cpp

+
+PdxRemoteWriterFactoryImpl::~PdxRemoteWriterFactoryImpl() = default;
+
+std::unique_ptr<PdxRemoteWriter> PdxRemoteWriterFactoryImpl::create(


It really seems like you could test this at unit level by mocking PdxTypeRegistery and PdxType. Make this a member function back from where it came and test it a the unit level. This avoids the creation of this factory, the exporting of internal symbols, and the interdiction of artificial behavior into the production implementation, via overriding this implementation.

Your approach seems promising. I'll look into it and see if it's feasible.

jake-at-work · 2022-02-22T23:42:14Z

This issue has been solved by using the previously fetched local PDX
type.

Do you mean this issue is solved in this PR or in a previous and this PR just adds more testing?

Ok, I think I found the fix but I a not sure I understand it. How does preserving the PdxType in all cases of the RemotePdxWriter construction fix the race condition?

gaussianrecurrence · 2022-02-23T18:25:23Z

This issue has been solved by using the previously fetched local PDX
type.

Do you mean this issue is solved in this PR or in a previous and this PR just adds more testing?

The issue I am referring to here is the one I described in the bullet above.

gaussianrecurrence · 2022-05-10T14:15:01Z

Closing this PR as this issue is already addressed in GEODE-10276

gaussianrecurrence force-pushed the feature/GEODE-9753 branch 4 times, most recently from af45bbf to 007e822 Compare November 6, 2021 03:42

gaussianrecurrence force-pushed the feature/GEODE-9753 branch from 007e822 to b010059 Compare November 8, 2021 21:04

gaussianrecurrence marked this pull request as ready for review November 9, 2021 07:06

pdxcodemonkey requested a review from jake-at-work February 9, 2022 20:16

pdxcodemonkey reviewed Feb 22, 2022

View reviewed changes

jake-at-work reviewed Feb 22, 2022

View reviewed changes

jake-at-work requested changes Feb 22, 2022

View reviewed changes

gaussianrecurrence closed this May 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-9753: Solve PDX serialization coredump #891

GEODE-9753: Solve PDX serialization coredump #891

gaussianrecurrence commented Nov 5, 2021 •

edited

gaussianrecurrence commented Jan 13, 2022

pdxcodemonkey Feb 22, 2022

pdxcodemonkey Feb 22, 2022

gaussianrecurrence Feb 23, 2022 •

edited

pdxcodemonkey Feb 22, 2022

gaussianrecurrence Feb 23, 2022

jake-at-work Feb 22, 2022

gaussianrecurrence Feb 23, 2022 •

edited

jake-at-work commented Feb 22, 2022

jake-at-work Feb 22, 2022

jake-at-work Feb 22, 2022

jake-at-work Feb 22, 2022

jake-at-work Feb 22, 2022

gaussianrecurrence Feb 23, 2022

jake-at-work Feb 23, 2022

jake-at-work Feb 22, 2022

jake-at-work Feb 22, 2022

gaussianrecurrence Feb 23, 2022

jake-at-work commented Feb 22, 2022

gaussianrecurrence commented Feb 23, 2022

gaussianrecurrence commented May 10, 2022


		PdxRemoteWriterFactoryImpl::~PdxRemoteWriterFactoryImpl() = default;

		std::unique_ptr<PdxRemoteWriter> PdxRemoteWriterFactoryImpl::create(

GEODE-9753: Solve PDX serialization coredump #891

GEODE-9753: Solve PDX serialization coredump #891

Conversation

gaussianrecurrence commented Nov 5, 2021 • edited

gaussianrecurrence commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaussianrecurrence Feb 23, 2022 • edited

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

gaussianrecurrence Feb 23, 2022 • edited

Choose a reason for hiding this comment

jake-at-work commented Feb 22, 2022

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

Choose a reason for hiding this comment

jake-at-work commented Feb 22, 2022

gaussianrecurrence commented Feb 23, 2022

gaussianrecurrence commented May 10, 2022

gaussianrecurrence commented Nov 5, 2021 •

edited

gaussianrecurrence Feb 23, 2022 •

edited

gaussianrecurrence Feb 23, 2022 •

edited