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

Introducing a client framework (shell & iopub clients) #37

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

anutosh491
Copy link
Collaborator

@anutosh491 anutosh491 commented Feb 16, 2024

Towards #32

@anutosh491 anutosh491 marked this pull request as draft February 16, 2024 09:42
src/xshell_client.hpp Outdated Show resolved Hide resolved
src/xclient_zmq_impl.hpp Outdated Show resolved Hide resolved
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

The more I think about it, the more I feel we should not split the control and the shell channels in the client. The pimpl is good for isolating zeromq, then what I see is:

  • an xheartbeat_client class, that runs its own thread, sending a "ping" message to the heartbeat each 100ms for instance
  • an xiopub_client class, that runs its own thread, polling the sub socket and pushing the received message into a queue. The reason for this is to avoid losing messages if a client waits too long and too many messages accumulate in the sub socket inernal queue
  • the shell and control socket can can be wrapped in a "xdealer_channel" (or any better name): they will share a common implementation and it makes sense to have them in a dedicated class rather than in xclient_zmq_impl

All these classes should send / receive zmq::multipart_t instead of xmessage, so that you don't have to store xauthentifcation in the "low level" classes.

The xclient_zmq_impl should provide the following methods:

void send_shell(xmessage msg);
std::optional<xmessage> receive_on_shell(timeout = infinite);
void register_on_shell(listener l);

// Same for control

std::size_t iopub_queue_size() const; // Not sure about this one
std::optional<xmessage> poll_iopub();
register_iopub_listener(listener l);

// Blocking call; this function will call one of the previously registered listeners
// depending on the received message (or if the iopub queue contains
// pending messages).
void wait_for_message();

It is responsible for serializing the messages before passing them to the dedicated channel, and responsible for deserializing them before returning them to the caller.

The client should also contain a client_messenger that will send stop messages to the heartbeat and the iopub threads.

src/xclient_zmq.cpp Outdated Show resolved Hide resolved
src/xclient_zmq.cpp Outdated Show resolved Hide resolved
src/xclient_zmq_impl.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xclient_zmq.cpp Outdated Show resolved Hide resolved
src/xclient_zmq.cpp Show resolved Hide resolved
src/xclient_zmq.cpp Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

I think you need an additional socket in xiopub_client to be connected to the xclient_messenger iopub_controller socket.

src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Outdated Show resolved Hide resolved
src/xiopub_client.cpp Outdated Show resolved Hide resolved
src/xiopub_client.cpp Outdated Show resolved Hide resolved
src/xclient_zmq_impl.hpp Outdated Show resolved Hide resolved
@anutosh491 anutosh491 requested a review from JohanMabille March 25, 2024 10:54
src/xclient_zmq.cpp Show resolved Hide resolved
src/xclient_zmq.cpp Show resolved Hide resolved
src/xclient_zmq_impl.cpp Show resolved Hide resolved
src/xclient_zmq_impl.cpp Show resolved Hide resolved
src/xclient_zmq_impl.cpp Show resolved Hide resolved
src/xclient_zmq_impl.cpp Outdated Show resolved Hide resolved
src/xclient_zmq_impl.hpp Outdated Show resolved Hide resolved
src/xdealer_channel.cpp Show resolved Hide resolved
xeus-zmqConfig.cmake.in Outdated Show resolved Hide resolved
@anutosh491 anutosh491 marked this pull request as ready for review March 26, 2024 10:45
@anutosh491 anutosh491 force-pushed the implement_xshell_client branch from 14f9877 to 157a987 Compare March 26, 2024 11:13
@JohanMabille JohanMabille merged commit ed541c0 into jupyter-xeus:main Mar 26, 2024
8 checks passed
@anutosh491 anutosh491 deleted the implement_xshell_client branch March 26, 2024 12:53
@anutosh491 anutosh491 changed the title [WIP] Adding support for xshell_client class Introducing a client framework (shell & iopub clients) Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants