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.

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

New ChannelMessageHandlers class (fixes issue #892) #894

Merged
merged 5 commits into from
Aug 25, 2022
Merged

New ChannelMessageHandlers class (fixes issue #892) #894

merged 5 commits into from
Aug 25, 2022

Conversation

ibc
Copy link
Member

@ibc ibc commented Aug 24, 2022

Details

Introduce a new ChannelMessageHandlers in worker that handles the Channel and PayloadChannel request/notification handlers.

Entities that wish to receive requests/notifications from Node/Rust must register themselves in their class constructor and unregister in their destructors.

class ChannelMessageHandlers
{
public:
	static void RegisterHandler(
	  const std::string& id,
	  Channel::ChannelSocket::RequestHandler* channelRequestHandler,
	  PayloadChannel::PayloadChannelSocket::RequestHandler* payloadChannelRequestHandler,
	  PayloadChannel::PayloadChannelSocket::NotificationHandler* payloadChannelNotificationHandler);
	static void UnregisterHandler(const std::string& id);

The RegisterHandler() function is smart and unregisters the entity from all maps in case its id was already present in any of them.

TODO

  • Adapt tests in Rust so they take into account the new channelRequestHandlers object field in the Worker dump output (I was not able to figure out how to compare the new obtained objects in Rust land).

@ibc ibc changed the title Working on issue #892 New ChannelMessageHandlers class (fixes issue #892) Aug 25, 2022
@ibc ibc marked this pull request as ready for review August 25, 2022 12:43
@ibc ibc requested review from nazar-pc and jmillan and removed request for nazar-pc August 25, 2022 12:44
@ibc
Copy link
Member Author

ibc commented Aug 25, 2022

@jmillan @nazar-pc this is ready for review. Unfortunately I need it merged today :(

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Makes sense to me, though I'm not a big fan of introduction of even more global data structures.

@ibc
Copy link
Member Author

ibc commented Aug 25, 2022

Makes sense to me, though I'm not a big fan of introduction of even more global data structures.

I would like to avoid those static maps and use a class instance instead, but that requires much more work and huge changes everywhere. But I'll think about it and will try to change it in the future. I won't forget.

@ibc
Copy link
Member Author

ibc commented Aug 25, 2022

@nazar-pc approved? XD

Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

With a heavy ❤️ (sorry 🦀, I'll fix you, I promise)

@ibc ibc merged commit df46fcc into v3 Aug 25, 2022
@ibc ibc deleted the fix-issue-892 branch August 25, 2022 13:06
Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants