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

[2.x] Major refactoring #447

Merged
merged 407 commits into from
Jan 23, 2021
Merged

[2.x] Major refactoring #447

merged 407 commits into from
Jan 23, 2021

Conversation

rennokki
Copy link
Collaborator

@rennokki rennokki commented Aug 13, 2020

Rewritten from scratch

The project got re-written from scratch as stated the reasons in #519

The contributors are going to remain the same, including @francislavoie with the awesome starting point to horizontally-scale the running servers.

Rewrite changelog

  • The migration file suffered a small modification. Run vendor:publish to get the second migration file that changes the fields to plural.
  • The configuration file got itself re-written as well with simplicity in mind (the initial 2.x was still a mess). Get a new copy of it.
  • Interfaces moved to Beyondcode\LaravelWebSockets\Contracts\*. If you extend certain functionalities, makes sure to use the right namespaced interfaces.
  • The replication driver is now called replication mode. Env's WEBSOCKETS_REPLICATION_MODE variable now can be set to local or redis.
  • Replicators got merged into Channel Managers. If you added your own custom replicator, you now should implement the BeyondCode\LaravelWebSockets\Contracts\ChannelManager interface.
  • Statistics Loggers are now called Statistics Collectors. Their job is to temporarily store the data between dumps into the database (by default).
  • Handlers for the WebSocket, as well as the HTTP API Server, got re-written. They are still found in the config, but make sure to check them if you extend one of them.
  • The vast majority functions of the channel managers now use PromiseInterfaces for consistencies. This means that instead of returning int or strings, you may now return a FulfilledPromise($value) to easily get resolved. If you don't implement your own channel manager (or now defunct, replicator), it's totally fine.
  • Presence channels now properly trigger the .here() methods.
  • Dashboard got broadcasting fixes. If you still encounter issues with sending events from the dashboard, open an issue.

Pre-Rewrite changelog

@lionslair
Copy link
Contributor

So So happy to see these things finally progressing. Will try and update this during the week and will test it out on staging.

@rennokki
Copy link
Collaborator Author

@lionslair Lovely!!! Hit the issues board for suggestions/issues.

@lionslair
Copy link
Contributor

@lionslair Lovely!!! Hit the issues board for suggestions/issues.

I will have not had a chance to enable yet. 2 / 12hr days. will do my best to get it turned on tomorrow. Thanks for putting the work in on these topics

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2020

Codecov Report

Merging #447 into master will increase coverage by 41.38%.
The diff coverage is 93.27%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master     #447       +/-   ##
=============================================
+ Coverage     48.57%   89.96%   +41.38%     
=============================================
  Files            50       60       +10     
  Lines          1126     1545      +419     
=============================================
+ Hits            547     1390      +843     
+ Misses          579      155      -424     
Impacted Files Coverage Δ Complexity Δ
src/Facades/WebSocketRouter.php 100.00% <ø> (ø) 0.00 <0.00> (?)
src/Server/HttpServer.php 100.00% <ø> (+100.00%) 0.00 <0.00> (-1.00) ⬆️
src/Server/Loggers/ConnectionLogger.php 0.00% <ø> (ø) 0.00 <0.00> (?)
src/Server/Loggers/HttpLogger.php 31.57% <ø> (ø) 0.00 <0.00> (?)
src/Server/Loggers/WebSocketsLogger.php 24.00% <ø> (ø) 0.00 <0.00> (?)
src/Server/Messages/PusherMessageFactory.php 100.00% <ø> (ø) 0.00 <0.00> (?)
src/Server/QueryParameters.php 100.00% <ø> (ø) 0.00 <0.00> (?)
src/Dashboard/Http/Controllers/SendMessage.php 34.78% <31.81%> (+34.78%) 0.00 <0.00> (-2.00) ⬆️
src/Server/Loggers/Logger.php 47.82% <47.82%> (ø) 0.00 <0.00> (?)
src/Server/HealthHandler.php 66.66% <66.66%> (ø) 0.00 <0.00> (?)
... and 102 more

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 c85ec38...2839d4c. Read the comment docs.

@rennokki rennokki marked this pull request as ready for review August 24, 2020 06:56
@lionslair

This comment has been minimized.

@rennokki
Copy link
Collaborator Author

@lionslair It seems like the event.error does not exist. Can you post here the value of event? 🤔

@lionslair
Copy link
Contributor

@lionslair It seems like the event.error does not exist. Can you post here the value of event? thinking

Solved. I edited the secret somehow this afternoon. I have spent all day trying to get this running on staging. I don't think the broadcaster websockets is working when using pubsub setup. I can see no messages in the dashboard being sent by laravel

@rennokki
Copy link
Collaborator Author

@lionslair The messages are being sent by the laravel's event classes or by using the dashboard form?

@lionslair
Copy link
Contributor

@lionslair The messages are being sent by the laravel's event classes or by using the dashboard form?

nope
Screenshot from 2020-08-24 16-18-18

@lionslair
Copy link
Contributor

@lionslair The messages are being sent by the laravel's event classes or by using the dashboard form?

nope
Screenshot from 2020-08-24 16-18-18

I get that message from friday still.

message: Argument 3 passed to BeyondCode\LaravelWebSockets\PubSub\Broadcasters\RedisPusherBroadcaster::broadcast() must be of the type array, null given, called in /home/PATH/vendor/beyondcode/laravel-websockets/src/Dashboard/Http/Controllers/SendMessage.php on line 41
Exception: TypeError
vendor/beyondcode/laravel-websockets/src/PubSub/Broadcasters/RedisPusherBroadcaster.php
line: 134

@lionslair
Copy link
Contributor

Hmm then others I do it but not showing in the activity. So I do not think the pubsub is working correctly.
Screenshot from 2020-08-24 16-25-40

@rennokki
Copy link
Collaborator Author

@lionslair Try updating to latest 2.0.0-beta.10, I have added more functional tests and there are fixed a few issues with the dashboard broadcasting & event listening.

@lionslair
Copy link
Contributor

@lionslair Try updating to latest 2.0.0-beta.10, I have added more functional tests and there are fixed a few issues with the dashboard broadcasting & event listening.

Been sticking to
"beyondcode/laravel-websockets": "2.x-dev",
apart from when I am trying to debug. just pulled

@lionslair
Copy link
Contributor

Screenshot from 2020-08-24 16-58-56

does it need to clear the channel and event name on every submit but leave the data. Can it leave it and have a reset button to clear it?

@lionslair
Copy link
Contributor

@lionslair Try updating to latest 2.0.0-beta.10, I have added more functional tests and there are fixed a few issues with the dashboard broadcasting & event listening.

Seems to post from the dashboard ok now but no events appear to be getting received from laravel that are being broadcast

@lionslair
Copy link
Contributor

@rennokki does it make any sense why or how $connection can be empty?
Screenshot from 2020-08-24 17-29-27

@rennokki
Copy link
Collaborator Author

If it's null then it uses the default Redis connection for the Pub/Sub:
Untitled

However, I think the connection name should be replaced with the one configured in the replication config.

@lionslair
Copy link
Contributor

If it's null then it uses the default Redis connection for the Pub/Sub:
Untitled

However, I think the connection name should be replaced with the one configured in the replication config.

Hmm we are using phpredis but I don't think that matters.

@rennokki
Copy link
Collaborator Author

rennokki commented Dec 8, 2020

This feature is the pong tracker that was implemented just like Pusher docs explains it (120s time to ping the connection): 55f1332
, actual logic here: https://github.com/beyondcode/laravel-websockets/blob/2.x/src/ChannelManagers/RedisChannelManager.php#L427-L441

The problem inflicted here is "what happens if the local connection suddenly drops"? If some VPS provider employee trips over a cable and the whole instance goes down without being able to SIGTERM/SIGINT the websockets process? The connections cannot be serialized to be safely stored, so-to-speak to have a stateful way of doing things. Once the connection falls down, it cannot be retrieved.

However, we do keep presence channels' users and we can know when the user last connected, but that requires more logic to cleanup any socket ids stored and their members data for sockets that are not found on any server.

…ts-disable

[fix] Prevent collecting stats if disabled
@francislavoie
Copy link
Contributor

That code is for handling dead websocket client connections. My suggestion was about dead websocket servers. All servers should keep an eye on whether each server has checked in with redis within a certain time period. If not, they should clear out any data that server was managing.

@zakriyarahman
Copy link

@rennokki Am I missing something?
image

@rennokki
Copy link
Collaborator Author

The artisan command websockets:flush was removed and we'll find a workaround in the next versions. Try upgrading to the latest beta version.

@rmundel
Copy link

rmundel commented Dec 29, 2020

Any more testing needed to release the kraken?

@rennokki
Copy link
Collaborator Author

rennokki commented Jan 8, 2021

Guess we're good to go. 👀

@morloderex
Copy link

morloderex commented Jan 15, 2021

Would it be worth it to wait for #644 sense it has many more breaking changes to app managers and and i personally would love to see you support MySql managers out of the box.

cc @mpociot

mahansky and others added 5 commits January 23, 2021 14:34
@rennokki
Copy link
Collaborator Author

@morloderex This PR has become too large to handle at this point, so maybe it's a good idea to schedule it for a possible 3.x, since 2.x brings great improvements that might be worth pushing.

@rennokki
Copy link
Collaborator Author

I have talked with @mpociot and decided to take Laravel's way of doing things:

  • we'll have 1.x support for the current master branch, so any apps that still use 1.x package will still be patched in case of security issues
  • 1.x will become the main branch for now, until a 2.x is released; upon release, 2.x branch is going to be the main branch
  • from now on, master will take only breaking changes commits for the upcoming versions (see https://laravel.com/docs/8.x/contributions#which-branch for more info)

@rennokki rennokki merged commit 1163078 into master Jan 23, 2021
@rennokki rennokki deleted the 2.x branch January 23, 2021 15:09
@radudiaconu0
Copy link

@rennokki i see you merged all tfrom 2.x branch into master branch nad delted 2.x branch. So how i install laravel-websockets 2 then ? :)

@rennokki
Copy link
Collaborator Author

@radudiaconu0 2.x is still under dev but i merged it into master to be able to handle the future prs better. This PR became too large to handle even for github (that threw unicorns when i tried to make a 2.x pr)

2.x is still available under latest beta.

@radudiaconu0
Copy link

radudiaconu0 commented Jan 25, 2021

@rennokki so when i run composer require beyondcode/laravel-websockets this should install 1.x branch or master? :)) ( i guess 1.x)

@rennokki
Copy link
Collaborator Author

It will install latest 1.x release. The 1.x release is still on the 1.x branch

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