New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugfix/intraprocess datasharing [10845] #1817

Merged

MiguelCompany merged 26 commits into master from bugfix/intraprocess-datasharing

Mar 30, 2021

Contributor

IkerLuengo commented Mar 9, 2021

This pull request solves the issues found when intraprocess and datasharing are mixed.

Removes the ACK for reusability of the payloads: Any payload that is not on the writer's history can be reused anytime
- This means that best effort behaves as such, but also that the reader may never catch up with the writer
Removes the notification back to the writer when a payload has been marked as reusable
Removes the payload locks related to the serialization. Instead, the sanity of the payload is checked before and after the deserialization

Contributor

richiprosima commented Mar 10, 2021

Build status:

Linux
Linux aarch64
Mac
Windows

IkerLuengo mentioned this pull request

[10730] For data-sharing delivery, subscriber loses messages while multiple publishers/subscribers are launched and one subscriber is terminated. #1782

Closed

IkerLuengo changed the title ~~Bugfix/intraprocess datasharing~~ Bugfix/intraprocess datasharing [10845]

Contributor

richiprosima commented Mar 15, 2021

Build status:

Linux
Linux aarch64
Mac
Windows

MiguelCompany requested changes

View reviewed changes

src/cpp/rtps/reader/StatefulReader.cpp Show resolved Hide resolved

src/cpp/rtps/DataSharing/ReaderPool.hpp Outdated Show resolved Hide resolved

src/cpp/rtps/reader/StatefulReader.cpp Outdated Show resolved Hide resolved

src/cpp/rtps/reader/StatefulReader.cpp Show resolved Hide resolved

src/cpp/rtps/reader/StatelessReader.cpp Outdated Show resolved Hide resolved

src/cpp/rtps/reader/WriterProxy.h Show resolved Hide resolved

src/cpp/rtps/writer/StatelessWriter.cpp Outdated Show resolved Hide resolved

test/unittest/dds/subscriber/DataReaderTests.cpp Outdated Show resolved Hide resolved

test/unittest/dds/subscriber/DataReaderTests.cpp Outdated Show resolved Hide resolved

test/unittest/dds/subscriber/DataReaderTests.cpp Outdated Show resolved Hide resolved

IkerLuengo force-pushed the bugfix/intraprocess-datasharing branch from 4630dc7 to 3df6020 Compare

March 16, 2021 10:43

MiguelCompany requested changes

View reviewed changes

Member

MiguelCompany left a comment

Apart from my comments below:

Fix uncrustify
There are some tests failing that I think should be related with the changes on this PR.
- Message [RTPS_READER_HISTORY Error] The Writer GUID_t must be defined -> Function eprosima::fastrtps::rtps::ReaderHistory::add_change on Windows throughput.performance.throughput.interprocess_best_effort_shm.data_loans_and_sharing is at least suspicious. It is also giving ThroughputTest: ../../../../src/fastrtps/src/cpp/rtps/history/TopicPayloadPool.cpp:97: virtual bool eprosima::fastrtps::rtps::TopicPayloadPool::get_payload(eprosima::fastrtps::rtps::SerializedPayload_t&, eprosima::fastrtps::rtps::IPayloadPool*&, eprosima::fastrtps::rtps::CacheChange_t&): Assertion cache_change.writerGUID != GUID_t::unknown()' failed.` on other platforms.
- PubSubHistory.PubSubAsReliableMultithreadKeepLast1.Datasharing is sending 300 samples, perhaps we need more buffer in the case of DataSharing
- PersistenceLargeData.PubSubAsReliablePubPersistentWithFrag.Datasharing seems to have returned samples in the wrong order.

src/cpp/fastdds/subscriber/DataReaderImpl/ReadTakeCommand.hpp Outdated Show resolved Hide resolved

src/cpp/fastdds/subscriber/DataReaderImpl/ReadTakeCommand.hpp Outdated Show resolved Hide resolved

src/cpp/rtps/DataSharing/ReaderPool.hpp Show resolved Hide resolved

src/cpp/rtps/reader/StatefulReader.cpp Outdated Show resolved Hide resolved

src/cpp/rtps/writer/StatelessWriter.cpp Outdated Show resolved Hide resolved

IkerLuengo force-pushed the bugfix/intraprocess-datasharing branch from 3df6020 to 49407c0 Compare

March 18, 2021 09:37

Contributor Author

IkerLuengo commented Mar 18, 2021

The traces on the failing PersistenceLargeData.PubSubAsReliablePubPersistentWithFrag.Datasharing suggests that data is missing (goes from sample 1 to sample 10 and nothing else). Cannot know when they were missing (on sending, on processing on the listener, on retrieving from the persistence DB).

I run the test locally 1000 times without fails. Let's wait for the CI run again.

IkerLuengo force-pushed the bugfix/intraprocess-datasharing branch 2 times, most recently from 714470e to b1cf2d2 Compare

March 22, 2021 15:58

MiguelCompany previously approved these changes

View reviewed changes

IkerLuengo dismissed MiguelCompany’s stale review via

March 23, 2021 14:56

IkerLuengo force-pushed the bugfix/intraprocess-datasharing branch from 4743664 to 562e58a Compare

March 23, 2021 14:57

MiguelCompany previously approved these changes

View reviewed changes

EduPonz added this to the v2.3.0 milestone

IkerLuengo added 14 commits

March 29, 2021 10:40


          Refs 10731. DataSharingListener provides access to the pool of a matc…

83d758a

…hed writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. The writer prioritizes intraprocess over datasharing deli…

b69e006

…veries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. The reader prioritizes intraprocess over datasharing deli…

2dbde27

…veries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Payloads not in the history of writer are considered reus…

516ab0d

…able

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Move override check on the reader to ReadTakeCommand

01a36f7

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Remove override check on best effort

c5810c7

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Check deserialization errors on reader

1fe0655

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Avoid lock on writer when getting payload

40a5362

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Test deserialization errors

270c73e

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Adapt DataSharing tests to new implementation

e395020

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Remove reusability notifications to writer

aad2c91

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Improve check of data validity on reader

de6478a

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Modify test to new behavior

d7acec8

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. fixup remove override check on RTPS

20ed930

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

IkerLuengo added 8 commits

March 29, 2021 10:44


          Refs 10731. fix mocks

45a8377

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. uncrustify

87cab85

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Suggested changes

78de1ee

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Keep datasharing compatibility on the writer info

aa78976

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. No error when requested loan size is zero

e54dc09

This corrects the regression on DataReaderTests.resource_limits

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Apply suggestions

8afe55c

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Atomic sequence number on datasharing node

f556e1f

- Make the sequence number atomic, as it signals the validity or
invalidation of the payload
- Clear the sequence number first when invalidating, set it last when
publishing
- Reset the pointer fields on the CacheChange when the pool returns no
valid data to the listener. Since the listener provides a
stack-allocated CacheChange for the pool to fill, if the return is
garbage, the destructor of the CacheChange will do unexpected things

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>


          Refs 10731. Protect the notification with the mutex

ad1ae2e

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

IkerLuengo dismissed MiguelCompany’s stale review via

ed06a0b

March 29, 2021 09:43

IkerLuengo force-pushed the bugfix/intraprocess-datasharing branch 2 times, most recently from ed06a0b to cce8358 Compare

March 29, 2021 11:04


          Refs 10731. Catch deserialization exceptions

4fa23f0

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

IkerLuengo force-pushed the bugfix/intraprocess-datasharing branch from cce8358 to 4fa23f0 Compare

March 29, 2021 11:08


          Refs 10731. Make sure linters do not complain of void returns

6856d29

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

MiguelCompany previously approved these changes

View reviewed changes

Member

MiguelCompany left a comment •

edited

Loading

LGTM. Please fix uncrustify.


          Refs 10731. Uncrustify

21d98a9

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

IkerLuengo dismissed MiguelCompany’s stale review via

21d98a9

March 29, 2021 11:27

MiguelCompany approved these changes

View reviewed changes

MiguelCompany requested changes

View reviewed changes

Member

MiguelCompany left a comment

Please fix warnings

src/cpp/fastdds/topic/TypeSupport.cpp Outdated Show resolved Hide resolved

src/cpp/fastdds/topic/TypeSupport.cpp Outdated Show resolved Hide resolved


          Refs 10731. Remove unused argument

10eca52

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

MiguelCompany approved these changes

View reviewed changes

MiguelCompany merged commit b555db3 into master

MiguelCompany deleted the bugfix/intraprocess-datasharing branch

March 30, 2021 07:17

IkerLuengo added a commit that referenced this pull request


          Datasharing delivery refactor

0462eea

This is a port of #1817 from <main> to <2.2.x>

* Refs 10731. DataSharingListener provides access to the pool of a matched writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. The writer prioritizes intraprocess over datasharing deliveries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. The reader prioritizes intraprocess over datasharing deliveries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Payloads not in the history of writer are considered reusable

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Move override check on the reader to ReadTakeCommand

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove override check on best effort

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Check deserialization errors on reader

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Avoid lock on writer when getting payload

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Test deserialization errors

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Adapt DataSharing tests to new implementation

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove reusability notifications to writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Improve check of data validity on reader

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Modify test to new behavior

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. fixup remove override check on RTPS

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. fix mocks

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. uncrustify

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Suggested changes

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Keep datasharing compatibility on the writer info

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. No error when requested loan size is zero

This corrects the regression on DataReaderTests.resource_limits

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Apply suggestions

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Atomic sequence number on datasharing node

- Make the sequence number atomic, as it signals the validity or
invalidation of the payload
- Clear the sequence number first when invalidating, set it last when
publishing
- Reset the pointer fields on the CacheChange when the pool returns no
valid data to the listener. Since the listener provides a
stack-allocated CacheChange for the pool to fill, if the return is
garbage, the destructor of the CacheChange will do unexpected things

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Protect the notification with the mutex

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Catch deserialization exceptions

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Make sure linters do not complain of void returns

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Uncrustify

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove unused argument

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

IkerLuengo added a commit that referenced this pull request


          Datasharing delivery refactor

c53b2b8

This is a port of #1817 from <main> to <2.2.x>

* Refs 10731. DataSharingListener provides access to the pool of a matched writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. The writer prioritizes intraprocess over datasharing deliveries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. The reader prioritizes intraprocess over datasharing deliveries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Payloads not in the history of writer are considered reusable

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Move override check on the reader to ReadTakeCommand

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove override check on best effort

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Check deserialization errors on reader

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Avoid lock on writer when getting payload

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Test deserialization errors

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Adapt DataSharing tests to new implementation

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove reusability notifications to writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Improve check of data validity on reader

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Modify test to new behavior

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. fixup remove override check on RTPS

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. fix mocks

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. uncrustify

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Suggested changes

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Keep datasharing compatibility on the writer info

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. No error when requested loan size is zero

This corrects the regression on DataReaderTests.resource_limits

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Apply suggestions

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Atomic sequence number on datasharing node

- Make the sequence number atomic, as it signals the validity or
invalidation of the payload
- Clear the sequence number first when invalidating, set it last when
publishing
- Reset the pointer fields on the CacheChange when the pool returns no
valid data to the listener. Since the listener provides a
stack-allocated CacheChange for the pool to fill, if the return is
garbage, the destructor of the CacheChange will do unexpected things

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Protect the notification with the mutex

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Catch deserialization exceptions

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Make sure linters do not complain of void returns

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Uncrustify

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove unused argument

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

MiguelCompany pushed a commit that referenced this pull request


          Datasharing delivery refactor (#1900)

5f93fec

This is a port of #1817 from <main> to <2.2.x>

* Refs 10731. DataSharingListener provides access to the pool of a matched writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. The writer prioritizes intraprocess over datasharing deliveries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. The reader prioritizes intraprocess over datasharing deliveries

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Payloads not in the history of writer are considered reusable

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Move override check on the reader to ReadTakeCommand

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove override check on best effort

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Check deserialization errors on reader

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Avoid lock on writer when getting payload

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Test deserialization errors

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Adapt DataSharing tests to new implementation

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove reusability notifications to writer

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Improve check of data validity on reader

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Modify test to new behavior

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. fixup remove override check on RTPS

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. fix mocks

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. uncrustify

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Suggested changes

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Keep datasharing compatibility on the writer info

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. No error when requested loan size is zero

This corrects the regression on DataReaderTests.resource_limits

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Apply suggestions

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Atomic sequence number on datasharing node

- Make the sequence number atomic, as it signals the validity or
invalidation of the payload
- Clear the sequence number first when invalidating, set it last when
publishing
- Reset the pointer fields on the CacheChange when the pool returns no
valid data to the listener. Since the listener provides a
stack-allocated CacheChange for the pool to fill, if the return is
garbage, the destructor of the CacheChange will do unexpected things

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Protect the notification with the mutex

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Catch deserialization exceptions

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Make sure linters do not complain of void returns

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Uncrustify

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

* Refs 10731. Remove unused argument

Signed-off-by: Iker Luengo <ikerluengo@eprosima.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet