Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

Allow timeout on handshake handler #12

Merged
merged 4 commits into from
Jul 22, 2019
Merged

Allow timeout on handshake handler #12

merged 4 commits into from
Jul 22, 2019

Conversation

kc7bfi
Copy link
Contributor

@kc7bfi kc7bfi commented Jul 6, 2017

Allow a timeout on the sync() call. This is to prevent the call from hanging and never returning.

@jenkinskurento
Copy link
Contributor

Hi there. Thanks for your PR.

I'm waiting for a Kurento member to verify that this patch is reasonable to test. If it is, they should reply with check out please on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

@jenkinskurento
Copy link
Contributor

There were no errors, go have a cup of coffee...

@kc7bfi
Copy link
Contributor Author

kc7bfi commented Sep 11, 2017

Any idea when this fix may be integrated into the main code?

@kc7bfi
Copy link
Contributor Author

kc7bfi commented Sep 11, 2017

I'm not sure how to split multiple commits into different pull requests. However, I've added code to name the async threads used in the WebSocket service class.

@kc7bfi
Copy link
Contributor Author

kc7bfi commented Nov 14, 2017

Any idea if an when this pull request can be added to the main development branch?

@kc7bfi
Copy link
Contributor Author

kc7bfi commented Nov 15, 2017

Found some more places where the sync() call can hang and never return

@nordri
Copy link
Contributor

nordri commented Nov 16, 2017

Hi @kc7bfi

The new Kurento team is now very small (after Twilio adquisition of most of developers) and we are very busy with other issues. Sorry.

@Forcada
Copy link

Forcada commented Jul 22, 2019

Any news about if this will be accepted and integrated into the main code?

Kurento client was hanging when trying to reconnect with the Kurento Media server and this pull request solved the problem.

@j1elo j1elo self-assigned this Jul 22, 2019
@j1elo j1elo requested a review from pabloFuente July 22, 2019 08:13
Copy link
Member

@pabloFuente pabloFuente left a comment

Choose a reason for hiding this comment

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

Fix seems to improve ws reconnection capabilities.

@j1elo
Copy link
Member

j1elo commented Jul 22, 2019

Sorry for the time it took, and thanks for the friendly reminder :-)
This gets in time to ship with release 6.11 In the end, the PR commit was (partially) reverted in 1827a7c

@j1elo j1elo merged commit 5a384ad into Kurento:master Jul 22, 2019
@j1elo
Copy link
Member

j1elo commented Jul 24, 2019

Hi, after some afterwards testing and discussion, we took the decision to have @pabloFuente rolling this change back (done in 1827a7c), because it introduces a fundamental change in how connections to the media server WS API were managed, which is in essence a breaking change in functionality.

With sync mode, in their initial connection applications know when KMS is not responding or not even started, with an immediate feedback due to failed connection. Changing to future-based async means that this case would need to wait for the timeout to happen, before getting a connection failure. This breaks some of our tests, and it will also break applications that expect the old behavior. We cannot break applications.

On the other hand, the sync code does indeed have the issue of sometimes hanging indefinitely while trying a reconnect. More info: Connection to KMS hangs. This should be treated as a bug in itself, and seek proper fixing of the issue, instead of completely changing the connection paradigm from synchronous to asynchronous.

@mitch2na
Copy link

@j1elo Added a new PR that is backwards compatible #18

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants