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

Allow multiple read from IPC + some refactor #102

Merged
merged 3 commits into from
Oct 23, 2023
Merged

Conversation

cylian914
Copy link
Contributor

The previous implementation was closing the connection right after 1 message (ligne 87) so i couldn't send multiple request with the same socket, this fix it.

@vaxerski
Copy link
Member

that is intentional though.

@cylian914
Copy link
Contributor Author

Why ?

@vaxerski
Copy link
Member

reading more can block, besides, there is no point. Just open a new socket connection?

@cylian914
Copy link
Contributor Author

cylian914 commented Oct 22, 2023

How can it block ? the only thing that could happen is the client crash right after the data is send (so client doesn't read hyprpaper message) and in that case it crash hyprpaper but it can also happen the old version. Also when the client disconnect it will send a message of 0 char and that is expected, with if (messageSize == 0) break; (line 61) it will close the socket connection. Nothing should block ?

@vaxerski
Copy link
Member

the client can send a message and then do nothing, blocking hyprpaper.

@cylian914
Copy link
Contributor Author

cylian914 commented Oct 22, 2023

its the same thing with the old one ? For the old one it will block at line 62 waiting for the data but the new one will block at line 59.

Also the old one could crash hyprpaper if you connect but immediately exit because it would try to write(ACCEPTEDCONNECTION, m_szReply.c_str(), m_szReply.length()); and having no connection to send (i didn't test it but it should do it)

@vaxerski
Copy link
Member

true that... Fine I guess. I'll review this tomorrow. One thing I can definitely say this MR needs is a clang-format

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@vaxerski vaxerski merged commit d6856ad into hyprwm:main Oct 23, 2023
1 check passed
vaxerski pushed a commit that referenced this pull request Jul 17, 2024
* Allow multiple read from IPC

* bring back old indentation

* format with clang-format

---------

Co-authored-by: cylian charbonnier <[email protected]>
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