Interprocessing fixes #3009
Interprocessing fixes #3009
Conversation
|
Hi @lampysprites! Thanks for your PR, I saw your extra changes in the other PR's comment and it's nice to have them here now. Didn't have enough time to advance with this. Was thinking about the extra complexity for the API but I think it might be useful to have these extra parameters. I'll try to merge in some way these changes tomorrow (maybe changing milliseconds by seconds as a number for |
|
Thanks @lampysprites! I've merged these changes with other fixes: 37a1b4b Mainly WebSocketMessageType constants now are in upper case (to match the style of all other constants, e.g. WebSocketMessageType.TEXT), and I've added a close_ws() to remove the WebSocket from g_connections and stop the g_timer in case that the WebSocket is GC'd. |
|
I still have a question about minreconnectwait/maxreconnectwait, what each parameter does? I saw this documentation but I'm still not sure what does each parameter (I'm asking this so we can write the documentation about it, and to know if both are needed). |
|
Thanks! Min and max just clamp the exponential time from both sides. The socket waits this time until the next attempt: // IXExponentialBackoff.cpp
uint32_t calculateRetryWaitMilliseconds(uint32_t retryCount,
uint32_t maxWaitBetweenReconnectionRetries,
uint32_t minWaitBetweenReconnectionRetries)
{
uint32_t waitTime = (retryCount < 26) ? (std::pow(2, retryCount) * 100) : 0;
if (waitTime < minWaitBetweenReconnectionRetries)
{
waitTime = minWaitBetweenReconnectionRetries;
}
if (waitTime > maxWaitBetweenReconnectionRetries || waitTime == 0)
{
waitTime = maxWaitBetweenReconnectionRetries;
}
return waitTime;
} |
|
Oh thanks for the explanation @lampysprites! 👍 About the deadlock fix, I had to reverted because it was giving a use-after-free error (we have to remove the DocUndo observer in the ~SpriteEvents dtor): |
|
Okay makes sense but it's troubling if the program keeps freezing. Do you have any ideas on how to avoid the deadlock then? Is it okay to only remove the last line that causes it? |
|
I'm going to investigate, I was able to reproduce the deadlock with the steps you gave. I think it's related to the |
…ocument Instead of using an onDestroy(Document), which can be called from the ClosedDocs background thread, we can disconnect all Doc observers on a new onCloseDocument() event called from Doc::close() (from the UI thread). This deadlock issue was commented here: #3009 (comment)
|
I think the deadlock issue should be fixed now: 4dcdcfe |
Hello again!
Out of three commits here, first two implement the solutions for connection backoff and update timer from the initial IPC PR (thanks!)
The last commit avoids a freeze that happened randomly (and regularly) when the tab, or the app is closed. As far as I can tell, it is a deadlock in
ClosedDocs::addClosedDoc's unique lock andSpriteEvents::~SpriteEventscall todoc()->remove_observer. Also noticed that for it to happen, the doc has to be released from ClosedDocs, doesnt matter if it's timed or immediately, otherwise the destructor is not called. Honestly I don't see what's wrong there.To reproduce:
Regarding the solution, I only removed the destructor involved. It seems that observer and event classes are destroyed together, and the global pointer clears in onDestroy, so no subscription remains anyway? Still I have a suspicion that the destructor was there for a reason.
With these changes, I don't run into any critical issues while doing half-duplex communication.
(For a while, the networking thread and this issue were the main suspect, perhaps it'll strike later:
machinezone/IXWebSocket#316)
I agree that my contributions are licensed under the Individual Contributor License Agreement V3.0 ("CLA") as stated in https://github.com/aseprite/sourcecode/blob/main/cla.md
I have signed the CLA following the steps given in https://github.com/aseprite/sourcecode/blob/main/sign-cla.md#sign-the-cla