Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

[2.x] Fix unsubscribeFromAllChannels leaving stale connections #708

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

simonbuehler
Copy link
Contributor

@simonbuehler simonbuehler commented Mar 4, 2021

the each-> call silently ran only until the first channel was a unable to unsubscribe the connection
leading to users on other channels seeming randomly not getting unsubscribed,
stale connections, a frustrated dev and a fishy smell all over the place.

after some serious hours of debugging and searching for this sneaky bug
the websocket-world is now a better place ;)

please merge ASAP, this is relevant for basically all setups that use more than one channel!
should fix e.g. #654, #657, #623 and propably #228,

edit:
fixed description

the `each->` call silently ran only for the first channel leading to
users on other channels seeming randomly not getting unsubscribed,
stale connections, a frustrated dev and a fishy smell all over the place.

after some serious hours of debugging and searching for this sneaky bug
the websocket-world is now a better place ;)
@codecov-io
Copy link

codecov-io commented Mar 4, 2021

Codecov Report

Merging #708 (6b6197f) into master (efb3aa8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #708      +/-   ##
============================================
+ Coverage     90.71%   90.73%   +0.01%     
  Complexity        2        2              
============================================
  Files            60       60              
  Lines          1691     1694       +3     
============================================
+ Hits           1534     1537       +3     
  Misses          157      157              
Impacted Files Coverage Δ Complexity Δ
src/ChannelManagers/LocalChannelManager.php 97.98% <100.00%> (+0.04%) 0.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efb3aa8...6b6197f. Read the comment docs.

@troy-whitespark
Copy link

Maybe not the right place for this, but if you don't mind sharing why exactly did this fix the issue?

I might be blind but aren't the changes just syntactic? What about higher order messages in collections was causing this to fail?

Thanks!

@simonbuehler
Copy link
Contributor Author

This is a good question but I didn't went down the rabbithole, i was too happy this finally worked.
Indeed would be interesting why this failed, maybe promise-related black magic?

@simonbuehler
Copy link
Contributor Author

@troy-whitespark @rennokki

if i understand it correctly the each gets a break when the closure returns false https://github.com/laravel/framework/blob/12b3d57c6f150e1b8def4535c347638113c22664/src/Illuminate/Collections/Traits/EnumeratesValues.php#L231

when unsubscription of a channel is not successfull

if (! $this->hasConnection($connection)) {
return false;
}

so thats the issue i guess - what do you think?

@i-rocky
Copy link

i-rocky commented Mar 15, 2021

I'm having the same issue and this PR does solve the problem.
@simonbuehler you're right. Returning false from Channel::unsubscribe() breaking the loop.

@simonbuehler
Copy link
Contributor Author

pls merge

@rennokki rennokki changed the title fix unsubscribeFromAllChannels / stale connections [2.x] Fix unsubscribeFromAllChannels leaving stale connections Mar 30, 2021
@rennokki rennokki merged commit 3cf74c7 into beyondcode:master Mar 30, 2021
AndyTWF added a commit to AndyTWF/uk-controller-api that referenced this pull request Apr 5, 2021
May address issues around scaling and stale connections
beyondcode/laravel-websockets#708
AndyTWF added a commit to VATSIM-UK/uk-controller-api that referenced this pull request Apr 10, 2021
May address issues around scaling and stale connections
beyondcode/laravel-websockets#708
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.

None yet

5 participants