Fix #892 #1303
Fix #892 #1303
Conversation
Removed additional event socket to reduce synchronisation problems and limited workspace update requests count.
| @@ -24,6 +24,10 @@ char *get_socketpath(void); | |||
| */ | |||
| int ipc_open_socket(const char *socket_path); | |||
| /** | |||
| * Issues a single IPC command without reading response. | |||
| */ | |||
| void ipc_single_command_no_response(int socketfd, uint32_t type, const char *payload, uint32_t *len); | |||
There was a problem hiding this comment.
Don't we need to read the response for further reads to make sense? Also, if chaning the client fixes this then I'm skeptical of the fix, malicious IPC clients shouldn't be able to crash sway.
There was a problem hiding this comment.
Response is being read by handle_ipc_event.
There was a problem hiding this comment.
Is there an advantage to reading it then instead of here? The response is generally meant to be specific to that request.
There was a problem hiding this comment.
Probably request rate should be limited on sway side too.
There was a problem hiding this comment.
We can probably avoid the deadlock without resorting to rate limiting, no? Is that really the problem?
There was a problem hiding this comment.
Alright then. Can you add the sway side to this PR?
There was a problem hiding this comment.
In i3bar there is one socket and requests made without reading response. Then, one handler handles events and responses.
There was a problem hiding this comment.
I'm not sure how to throttle on sway side. Since all requests processed synchronous, we cannot limit request count (without counting requests in last n seconds), but we could make socket non-blocking, which will help with locks, but on high request rates there may be unexpected problems with data loss.
There was a problem hiding this comment.
Data loss sounds better than crashing to me.
|
Yeah, this needs to be fixed at the sway level, though I'm not opposed to throttling swaybar too. |
This should prevent deadlocks with malicious clients
|
Looks like after making socket non-blocking scrolling works fine without throttling on swaybar side. But with two sockets it still hangs. |
|
I figured that might work. Can you rollback the other changes? |
|
Well, most changes are still needed (it hangs with event socket), but I can rollback throttling. |
|
Well, throttling is fine. I'm still not on board with ipc_single_command_no_response. |
|
Okay, I'll try to fix it with two sockets and without _no_response, but I'm not sure if it will work. |
|
I've played a bit with nonblocking sockets on sway side. Data loss could lead to client crashes, for example if ipc response lost. Probably it will be okay with throttling on client side, but nonblocking I/O may cause other unexpected bugs with current implementation. Generally, the problem is that if client subscribed to some events and not reading them (e.g. swaybar not reading from event socket while processing scroll events), socket buffer fills and then on next Example |
|
Ah, and non-blocking patch is pretty simple. Link |
|
Do you also have to deal with E_AGAIN? |
|
Uh huh. I've never worked with async IO before, though. |
|
grep for nonblock in the read(3) man page, it's pretty straightforward. Are you going to update this PR? |
|
Yeah, I'll figure it out. No, I'll make another one. |
|
Sounds good. Many thanks! |
Removed additional event socket to reduce synchronization problems
and limited workspace update requests count.