Closed ven0ms99 closed 3 years ago
Do you have any logs available? Without that it will be a bit hard to diagnose.
Bump
@Hillcow It happened to me, your issue is likely that the server ran out of memory, if that happens all request will fail because php cannot allocate more memory but the server will still be considered running (as the process doesn't exit if the failed allocation is within a try, catch) so supervisor will not restart it
Solution 1: increase the allowed memory for the ws server, this is a missing option in this lib that could be done with ini_set but can also be done when launching the command using php -d MEMORY_LIMIT=1G
(doesn't work on all setups)
Solution 2: make sure to catch allocation failure exception and exit the process when that happens
@Tofandel I assume Solution 2 can be adapted directly in this package by using a signal listener: https://github.com/reactphp/event-loop#addsignal
The bad thing is that that it needs the pcntl extension, so Windows is not favored for this kind of change.
The weird thing is that the loop kept running when out of memory, that's a fatal error so it shouldn't be caught in try catch but somehow the process still managed to continue and in the on error handler of the logger you can see the caught messages of "Unable to allocate xxx bytes" multiple times, once after each request
We resolved by restarting the process every week, very ugly but it's the only working solution for us as memory was below the max limit. Use also used pcntl extension as, by default, the worst loop was in use.
Ugly-ugly, but we gotta find what causes this huge increase in memory. Prolly some memory leaks caused by closed connections? 🤔
@rennokki I don't really see how this would solve the issue as that would listen for external signals (eg: sigterm, sigend) and not internal unrecoverable errors
Yes there likely is some memory leak in this package I would suggest looking into the statistics part, last week I completely disabled the statistics by modifying some code because I didn't need them and didn't need the table as well, there is no memory increase over time anymore Though I also added a gc_collect_cycles everytime a connection closes, so it might also be some cyclic reference issue and if the garbage collector is not called enough and you get a high volume of request before garbage collection (eg 200 requests in 5 minutes) then the memory just blows
There is a lot of circular references eg ->app = app()
in all the loggers singleton so I wouldn't be surprised
I understand that PHP cannot create exceptions because the memory limit is... well, memory limit. So maybe we can check with a timer if the total ram reached a percentage in the last x minutes and restart the server.
Again, if this seems to be due to statistics, i understand. There was a PR merged for 1.6.1 that fixes a decorator for websockets by cloning the self. Perhaps it needs some on the other decorators.
@rennokki Yeah that was me :p Well actually I think for loggers, singleton should be dropped at least for the message logger and instead a trait or abstract class should be added, it's a big BC but the websocket logger is causing other issues by decorating the class
One of them is interface implementation, ratchet will check your class for some implementations and provide some features in that case (Eg: supported protocol with interface WsServerInterface
) and by decorating you lose the interface and so the feature, I think logging should be done within the class by extension of onMessage and onError rather than wrapping the class within a class
For memory you can indeed add a loop ticker every 30sec and check if you are at more than 95% of ini_get('memory_limit')
if so stop the server
I can drop the decorators for the 2.x. It is still in beta and it needs thoroughly tested for this point.
I basically managed to replicate it:
add this one to the start command:
$this->loop->addPeriodicTimer(1, function () {
$memoryInMb = round(memory_get_usage() / 1048576, 2);
$this->line("Using: {$memoryInMb} MB");
});
php artisan websockets:serve
Managed to solve the issue by working on the save()
function from the statistic manager and add a $this->statistics = [];
after the for each loop, but it still looks like it keeps something in-memory, then it drops again back to normal after a while.
@rennokki make sure to call gc_collect_cycles() before displaying the memory or you might see inaccurate result because of circular references then check if memory is still increasing
@Tofandel https://github.com/beyondcode/laravel-websockets/pull/475/commits/ed96e24f6a83bdeb7cc9d57a24610edbcd790410 seems to fix the auto-incrementing of memory, but after the gc_collect_cycles(), seems that the memory increases incrementally on each re-connection using Echo.
1 sec tick, both statistics and memory count. Refreshed the page on each increase:
16.63 -> 17.73 -> 17.82 -> 17.91 -> 18.00 -> 18.09 -> 18.15 -> (closed the connection; no active connections at this point) -> 18.12 -> 18.04 -> 17.95 -> 17.77 -> 17.68
The base start is 16.63, and 17.68 is the last value and it's been like 3 mins and it still stays on 17.68, so it seems like you are right, there is some cycle that keeps being beaten up every time a new connection comes in. Maybe I can do this at scale and try to initiate multiple connections.
Switching up on 2.x to \BeyondCode\LaravelWebSockets\Statistics\Logger\NullStatisticsLogger::class
stops any leakage, so it's not the singleton itself, it's the code within that breaks things up. 🤔
This thing here messes up the memory:
$this->browser
->post(
action([WebSocketStatisticsEntriesController::class, 'store']),
['Content-Type' => 'application/json'],
stream_for(json_encode($postData))
);
@Tofandel Seems like the controller caused the issue. Removed it in https://github.com/beyondcode/laravel-websockets/pull/475/commits/99b55411c1666a9518fbcaf1e0f80c87be33fb86 and it seems like the issue is gone, although it still increments after some time with 0.01 MB, like every 5th or 6th connection adds a 0.01 MB to the memory out of nowhere. 🤔
@rennokki that seems like a normal increase, some laravel helpers have a local cache to lookup singletons/controllers, convert strings etc, likely what you are seeing
Won't you believe, but broadcast(new StatisticsUpdated(...))
is causing the issue. :/ Now I'm really confused. Disabling it would turn the memory consumption back to normal. More specific, this: https://github.com/beyondcode/laravel-websockets/pull/475/files#diff-ed8111d433470eea09c12aede397e6e2R108-R110
Disabling decorators has nothing to do with the memory leak, I can confirm it.
I removed the event broadcasting that caused the issue with an automatic polling function in the dashboard. It keeps polling the /statistics endpoint for refreshes, allowing to disable or refresh on-demand.
Isn't the memory leak caused by the broadcast function? If so the memory leak is within laravel and the issue is more serious
👀
After, this package is hooking into the broadcast function to send websocket messages correct? In that case it might be wise to do some xdebugging and see where that function is going and what data it attaches, as it can either be in laravel or just in the broadcasting part of this lib
Edit: I'm looking into it
@rennokki I figured it out, the event you removed doesn't implement the ShouldBroadcastNow
interface, as such it is added to a null queue and somehow kept in memory but broadcasted right away for some reason, something weird is going on, in any case changing ShouldBroadcast
to ShouldBroadcastNow
should fix it
@Tofandel That means that although the driver is by default sync
and the event gets triggered, the method still causes a memory leak.
@Tofandel The memory leak still persists, even with ShouldBroadcastNow
. Am i missing something?
@rennokki Debugging a little more, btw unused variable there, not that it's related
public function broadcastOn()
{
$channelName = Str::after(DashboardLogger::LOG_CHANNEL_PREFIX.'statistics', 'private-');
return new PrivateChannel(
Str::after(DashboardLogger::LOG_CHANNEL_PREFIX.'statistics', 'private-')
);
}
@Tofandel The problem is a bit tricky. Until we don't get a confirmation that https://github.com/laravel/framework/issues/33952 is caused by Laravel via the broadcast() method, I assume changes from https://github.com/beyondcode/laravel-websockets/pull/475 to remove the broadcaster and add the auto-polling are completely valid.
It is a bit weird to call it like the leak is from this package because the problems start with calling the broadcast()
method. 🤔
Gonna merge it for 2.x and see what happens afterward.
@Tofandel So it seems like Laravel classified the issue as a bug 🤔 https://github.com/laravel/framework/issues/33952
@rennokki FYI, you should not be broadcasting inside of ReactPHP. That ends up using the synchronous Redis client (predis/phpredis) instead of the ReactPHP redis client which is async, non-blocking, and safe for use in the ReactPHP server loop.
@mpociot pointed out this and going to be removed before 2.x gets released.
The issue was caused by the Ignition package, see the quick-fix and follow-up coming from the Ignition team: https://github.com/beyondcode/laravel-websockets/issues/379#issuecomment-736716734
@rennokki I don't actually see a fix anywhere? Following all the links, all I see are MRs that were closed/rejected for various reasons, none merged.
@francislavoie This specific comment: https://github.com/laravel/framework/issues/33952#issuecomment-693350705
Hey there, for me the problem unfortunately still persists. Laravel websockets randomly stops working and supervisor does not restart the process as it does not crash. I've looked through supervisor logs, Nginx logs, Laravel logs, but there is no sign of any failure.
So basically it stopped working, even though the process in supervisord was showing up as running (since 60 days). After I restarted the process with supervisord it worked again. What could have happened and how could I avoid this in the future?