Skip to content
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

Iox #615 block publisher when subscriber queue full #663

Merged
merged 70 commits into from Apr 3, 2021
Merged

Iox #615 block publisher when subscriber queue full #663

merged 70 commits into from Apr 3, 2021

Conversation

elfenpiff
Copy link
Contributor

@elfenpiff elfenpiff commented Mar 31, 2021 •
edited

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Branch follows the naming format (iox-#123-this-is-a-branch)
  4. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  5. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  6. Relevant issues are linked
  7. Add sensible notes for the reviewer
  8. All checks have passed (except task-list-completed)
  9. Assign PR to reviewer

Notes for Reviewer

  • Enables the user to implement blocking publishers, which do a busy wait till the subscriber has fetched some data

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 add option for publisher and subscriber
509dd17 
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@mossmaurice
iox-eclipse-iceoryx#615 Mark places on where to store and compare Que…
0d5eabd 
…ueFullPolicy

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
iox-eclipse-iceoryx#615 Add delivery queue full policy to base port a…
37f84eb 
…nd let RouDi compare it in the discovery loop
@elfenpiff
iox-eclipse-iceoryx#615 integrated trigger queue
1adac39 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
Merge branch 'iox-eclipse-iceoryx#615-block-publisher-when-subscriber…
862faa4 
…-queue-full' of github.com:ApexAI/iceoryx into iox-eclipse-iceoryx#615-block-publisher-when-subscriber-queue-full
@mossmaurice
iox-eclipse-iceoryx#615 Extend base port test
fe97a88
@elfenpiff
iox-eclipse-iceoryx#615 trigger queue integrated in variant queue
456103d 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
Merge branch 'iox-eclipse-iceoryx#615-block-publisher-when-subscriber…
3cd2360 
…-queue-full' of github.com:ApexAI/iceoryx into iox-eclipse-iceoryx#615-block-publisher-when-subscriber-queue-full
@elfenpiff
iox-eclipse-iceoryx#615 added blocking test and adjusted overflow test
3d10c08 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 refactored and cleaned up variant queue
5106d7a 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@mossmaurice
iox-eclipse-iceoryx#615 Extend PortManager test
ea439c2 
Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@mossmaurice
iox-eclipse-iceoryx#615 Move blocking policies from BasePort to Publi…
ca99639 
…sher/SubscriberPort

Signed-off-by: Simon Hoinkis <simon.hoinkis@apex.ai>
@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 extend getMiddlewareSubscriber/Publisher and …
debdfcb 
…processMessage,

add tests

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@elfenpiff
iox-eclipse-iceoryx#615 wrong value returned on overflowing push in 1…
f558940 
…:n sofi

Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
Merge branch 'iox-eclipse-iceoryx#615-block-publisher-when-subscriber…
ca58f0e 
…-queue-full' of github.com:ApexAI/iceoryx into iox-eclipse-iceoryx#615-block-publisher-when-subscriber-queue-full
@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 rename base options
22e143f 
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@FerdinandSpitzschnueffler
Merge branch 'iox-eclipse-iceoryx#615-block-publisher-when-subscriber…
97c6be7 
…-queue-full' of github.com:ApexAI/iceoryx into iox-eclipse-iceoryx#615-block-publisher-when-subscriber-queue-full
@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 fix include paths
2db16b4 
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@budrus
iox-eclipse-iceoryx#615 invalid assumption for subscription state in …
fd76d71 
…PortMangerTest

Signed-off-by: Michael Poehnl <michael.poehnl@apex.ai>
@budrus
iox-eclipse-iceoryx#615 extension of publisher port test
6473c0b 
Signed-off-by: Michael Poehnl <michael.poehnl@apex.ai>
@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 add QueueFullPolicy to C subscriber options
1fbb3ba 
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 add SubscriberTooSlowPolicy to C publisher op…
0c63966 
…tions

Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@FerdinandSpitzschnueffler
iox-eclipse-iceoryx#615 add cpp2c enum translation
bfdf8c8 
Signed-off-by: Marika Lehmann <marika.lehmann@apex.ai>
@elfenpiff
iox-eclipse-iceoryx#615 merged master and integration test for comple…
2a1b8d3 
…x data types

Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 forwarding policy settings to chunk distributor
a326144 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 blocking push implemented in chunk distributor
7809630 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 code cleanup
810bf4d 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 implemented blocking options and verified it …
5b8f980 
…with an integration test

Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 chunk queue pusher push returns bool tested
d91620a 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 tested blocking in chunk distributor
8821d6e 
Signed-off-by: Christian Eltzschig <me@elchris.org>
@elfenpiff
iox-eclipse-iceoryx#615 removed advanced LSAN options to solve mac os…
79aa02e 
… libc++abi.dylib: terminating

Signed-off-by: Christian Eltzschig <me@elchris.org>
@budrus budrus mentioned this pull request Apr 2, 2021
Closed
@budrus budrus changed the base branch from master to release_1.0 April 3, 2021 10:02
@elBoberido elBoberido changed the base branch from release_1.0 to master April 3, 2021 10:34
@elBoberido elBoberido changed the base branch from master to release_1.0 April 3, 2021 10:34
@dkroenke @elBoberido
iox-eclipse-iceoryx#670 build Pull-Requests for release branch
00952ba 
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
@dkroenke @elBoberido
iox-eclipse-iceoryx#670 add commata
fa1cca8 
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
budrus
budrus reviewed Apr 3, 2021
View reviewed changes
iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_queue_pusher.inl Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/internal/popo/ports/client_port_data.hpp
@@ -38,7 +38,8 @@ struct ClientPortData : public BasePortData
mepoo::MemoryManager* const memoryManager,
const mepoo::MemoryInfo& memoryInfo = mepoo::MemoryInfo()) noexcept;


static constexpr SubscriberTooSlowPolicy CLIENT_SUBSCRIBER_POLICY = SubscriberTooSlowPolicy::DISCARD_OLDEST_DATA;
static constexpr QueueFullPolicy CLIENT_PUBLISHER_POLICY = QueueFullPolicy::DISCARD_OLDEST_DATA;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel we should have new enums for clients and servers. They are reusing ChunkSender and ChunkReceiver but have nothing to do with Publisher and Subscriber

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no blocker, as client and server is not yet fully implemented. So this can be changed later

iceoryx_posh/include/iceoryx_posh/internal/popo/ports/server_port_data.hpp
@@ -38,7 +38,8 @@ struct ServerPortData : public BasePortData
mepoo::MemoryManager* const memoryManager,
const mepoo::MemoryInfo& memoryInfo = mepoo::MemoryInfo()) noexcept;


static constexpr SubscriberTooSlowPolicy SERVER_SUBSCRIBER_POLICY = SubscriberTooSlowPolicy::DISCARD_OLDEST_DATA;
static constexpr QueueFullPolicy SERVER_PUBLISHER_POLICY = QueueFullPolicy::DISCARD_OLDEST_DATA;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same same

iceoryx_utils/include/iceoryx_utils/internal/cxx/variant_queue.inl Outdated Show resolved Hide resolved
@elBoberido
iox-eclipse-iceoryx#615 make timing test more robust
ad61aad 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#615 make another timing test more robust
29f6a6d 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
elBoberido
elBoberido reviewed Apr 3, 2021
View reviewed changes
iceoryx_utils/include/iceoryx_utils/internal/concurrent/trigger_queue.inl
{
return m_queue->empty();
return m_queue.pop();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make the ActiveObject::mainLoop burn the CPU since it relied on this to be blocking.

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_distributor.inl Outdated Show resolved Hide resolved
iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_distributor.inl
Comment on lines +164 to +165
std::sort(getMembers()->m_queues.begin(), getMembers()->m_queues.end());
std::sort(remainingQueues.begin(), remainingQueues.end());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also be optimized if we sort the queues once when they are subscribed

iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_distributor.inl Show resolved Hide resolved
elBoberido and others added 8 commits April 3, 2021 14:18
@elBoberido
iox-eclipse-iceoryx#615 small optimization in hot path
b0ce085 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@budrus
iox-eclipse-iceoryx#615 only report missed data on subscriber side wh…
b7d3298 
…en chunks are lost

Signed-off-by: Michael Poehnl <michael.poehnl@apex.ai>
@budrus
Merge branch 'iox-eclipse-iceoryx#615-block-publisher-when-subscriber…
a87c87c 
…-queue-full' of https://github.com/ApexAI/iceoryx into iox-eclipse-iceoryx#615-block-publisher-when-subscriber-queue-full
@elBoberido
iox-eclipse-iceoryx#615 remove blocking options from variant queue
d0b7a25 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido
iox-eclipse-iceoryx#615 remove remainders of blocking options from va…
596d9a1 
…riant queue

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@budrus
iox-eclipse-iceoryx#615 only cleanup lost chunk handling
7f260e0 
Signed-off-by: Michael Poehnl <michael.poehnl@apex.ai>
@budrus
Merge branch 'iox-eclipse-iceoryx#615-block-publisher-when-subscriber…
56f9f36 
…-queue-full' of https://github.com/ApexAI/iceoryx into iox-eclipse-iceoryx#615-block-publisher-when-subscriber-queue-full
@elBoberido
iox-eclipse-iceoryx#615 fix build failure due to missing include
600aa77 
Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>
@elBoberido elBoberido force-pushed the iox-#615-block-publisher-when-subscriber-queue-full branch from 069ff11 to 600aa77 Compare April 3, 2021 13:37
@budrus budrus self-requested a review April 3, 2021 14:02
@budrus budrus merged commit a429050 into eclipse-iceoryx:release_1.0 Apr 3, 2021
@elBoberido elBoberido mentioned this pull request Apr 3, 2021
Open
9 tasks
marthtz pushed a commit to boschglobal/iceoryx that referenced this pull request May 12, 2021
@budrus @marthtz
Merge pull request eclipse-iceoryx#663 from ApexAI/iox-#615-block-pub…
bd2e306 
…lisher-when-subscriber-queue-full

Iox eclipse-iceoryx#615 block publisher when subscriber queue full
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@budrus budrus budrus approved these changes

@elBoberido elBoberido elBoberido approved these changes

@dkroenke dkroenke Awaiting requested review from dkroenke

@mossmaurice mossmaurice Awaiting requested review from mossmaurice

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler Awaiting requested review from FerdinandSpitzschnueffler

Assignees

@elfenpiff elfenpiff

Labels
enhancement New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possibility to block the publisher when subscriber queue is full
6 participants
@elfenpiff @mossmaurice @dkroenke @budrus @elBoberido @FerdinandSpitzschnueffler