From f3c27bf6515625f404a9039d999bff9751d40e3d Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Mon, 14 Apr 2025 06:49:03 -0500 Subject: [PATCH 1/2] Fix memory leaks. It turns out that none of the ContentGenerator controller objects are being destroyed when a request finishes. So each hypnotoad process (or morbo in development) has every ContentGenerator controller object for every request it renders saved in memory until the process ends. That means that everything it had a reference to is also saved in memory. That includes a `WeBWorK::CourseEnvironment` instance, a `WeBWorK::Authen` instance, a `WeBWorK::Authz` instance, and a `WeBWorK::DB` instance (and everything it has a reference to). Furthermore, even if the controller objects are destroyed and the `WeBWorK::DB` instance with it, none of the `WeBWorK::DB::Schema` instances (one for each table) are ever destroyed. There are two things that cause these references to be kept when they shouldn't be. The first is the more obvious circular reference.. A `WeBWorK::Controller` object (from with the `WeBWorK::ContentGenerator` modules derive) keeps a reference to a `WeBWorK::Authz` instance, and that instance keeps a reference back to the controller. However, the `WeBWorK::Authz` doesn't weaken the reference back to the controller. That was my fault in the conversion to Mojolicious I commented out the `weaken` statement that prevented this circular reference. That was because in the initial conversion the controller didn't have a reference to the `WeBWorK::Authz` instance, and so it was going out of scope and causing problems. However, when the reference to that instance was added that should have been uncommented. Another case of this is that `WeBWorK::Authen::LTIAdvanced` and `WeBWorK::Authen::LTIAdvantage` packages were keeping a circular reference to the controller as well. The new methods in those packages was just deleted so that they use the `WeBWorK::Authen` new method which already does the right thing. A third case occurs with the `WeBWorK::DB` instance and the `WeBWorK::DB::Schema` instances both of which hold references to each other. The other thing that causes an extra reference to be kept is an anonymous subroutine (or closure) using an instance. In this case Perl forces the instance to be kept in scope for usage in the closure. The global `$SIG{__WARN__}` handler defined in `Mojolicious::WeBWorK` uses the `$c` controller instance, and that is what prevents the `WeBWorK::ContentGenerator` modules from going out of scope. So that instance in the `around_action` hook needs to be weakened. For the `WeBWorK::DB::Schema::NewSQL::Std` and `WeBWorK::DB::Schema::NewSQL::Merge` objects the issue is the `transform_table` and `transform_all` closures for the sql abstract instances. Those prevent the schema objects from going out of scope and so the `$self` in the `sql_init` methods where those closures are defined needs to be weakened as well. --- lib/Mojolicious/WeBWorK.pm | 5 ++++- lib/WeBWorK/Authen/LTIAdvanced.pm | 23 ----------------------- lib/WeBWorK/Authen/LTIAdvantage.pm | 16 ---------------- lib/WeBWorK/Authz.pm | 2 +- lib/WeBWorK/ContentGenerator/Home.pm | 1 - lib/WeBWorK/DB/Schema.pm | 4 ++++ lib/WeBWorK/DB/Schema/NewSQL/Merge.pm | 3 +++ lib/WeBWorK/DB/Schema/NewSQL/Std.pm | 3 +++ 8 files changed, 15 insertions(+), 42 deletions(-) diff --git a/lib/Mojolicious/WeBWorK.pm b/lib/Mojolicious/WeBWorK.pm index 20cac99367..8bbbf8bd24 100644 --- a/lib/Mojolicious/WeBWorK.pm +++ b/lib/Mojolicious/WeBWorK.pm @@ -24,7 +24,8 @@ Mojolicious::WeBWorK - Mojolicious app for WeBWorK 2. use Env qw(WEBWORK_SERVER_ADMIN); -use Mojo::JSON qw(encode_json); +use Mojo::JSON qw(encode_json); +use Scalar::Util qw(weaken); use WeBWorK; use WeBWorK::CourseEnvironment; @@ -146,6 +147,8 @@ sub startup ($app) { around_action => async sub ($next, $c, $action, $last) { return $next->() unless $c->isa('WeBWorK::ContentGenerator'); + weaken $c; + my $uri = $c->req->url->path->to_string; $c->stash->{warnings} //= ''; diff --git a/lib/WeBWorK/Authen/LTIAdvanced.pm b/lib/WeBWorK/Authen/LTIAdvanced.pm index b7dd57027e..853040d4db 100644 --- a/lib/WeBWorK/Authen/LTIAdvanced.pm +++ b/lib/WeBWorK/Authen/LTIAdvanced.pm @@ -40,29 +40,6 @@ use WeBWorK::Authen::LTIAdvanced::Nonce; $Net::OAuth::PROTOCOL_VERSION = Net::OAuth::PROTOCOL_VERSION_1_0A; -=head1 CONSTRUCTOR - -=over - -=item new($c) - -Instantiates a new WeBWorK::Authen object for the given WeBWorK::Controller ($c). - -=cut - -sub new { - my ($invocant, $c) = @_; - my $class = ref($invocant) || $invocant; - my $self = { c => $c, }; - #initialize - bless $self, $class; - return $self; -} - -=back - -=cut - ## this is only overridden for debug logging #sub verify { # debug("BEGIN LTIAdvanced VERIFY"); diff --git a/lib/WeBWorK/Authen/LTIAdvantage.pm b/lib/WeBWorK/Authen/LTIAdvantage.pm index 3927584e2e..0062fc0499 100644 --- a/lib/WeBWorK/Authen/LTIAdvantage.pm +++ b/lib/WeBWorK/Authen/LTIAdvantage.pm @@ -33,22 +33,6 @@ use WeBWorK::Utils::DateTime qw(formatDateTime); use WeBWorK::Utils::Instructor qw(assignSetToUser); use WeBWorK::Authen::LTIAdvantage::SubmitGrade; -=head1 CONSTRUCTOR - -=over - -=item new($c) - -Instantiates a new WeBWorK::Authen object for the given WeBWorK::Controller ($c). - -=back - -=cut - -sub new ($invocant, $c) { - return bless { c => $c }, ref($invocant) || $invocant; -} - sub request_has_data_for_this_verification_module ($self) { debug('LTIAdvantage has been called for data verification'); my $c = $self->{c}; diff --git a/lib/WeBWorK/Authz.pm b/lib/WeBWorK/Authz.pm index eed66350ed..4551e7a1e1 100644 --- a/lib/WeBWorK/Authz.pm +++ b/lib/WeBWorK/Authz.pm @@ -84,7 +84,7 @@ sub new { my ($invocant, $c) = @_; my $class = ref($invocant) || $invocant; my $self = { c => $c, }; - #weaken $self->{c}; + weaken $self->{c}; $c->{permission_retrieval_error} = 0; bless $self, $class; diff --git a/lib/WeBWorK/ContentGenerator/Home.pm b/lib/WeBWorK/ContentGenerator/Home.pm index 3cfea1c8ce..a403a334b1 100644 --- a/lib/WeBWorK/ContentGenerator/Home.pm +++ b/lib/WeBWorK/ContentGenerator/Home.pm @@ -23,7 +23,6 @@ WeBWorK::ContentGenerator::Home - display a list of courses. =cut use WeBWorK::Utils::Files qw(readFile); -use WeBWorK::Localize; sub info ($c) { my $result; diff --git a/lib/WeBWorK/DB/Schema.pm b/lib/WeBWorK/DB/Schema.pm index 5116f8069f..7dd95e2acc 100644 --- a/lib/WeBWorK/DB/Schema.pm +++ b/lib/WeBWorK/DB/Schema.pm @@ -81,6 +81,8 @@ dependent. C<$db> is provided so that schemas can query other schemas. =cut +use Scalar::Util qw(weaken); + sub new { my ($proto, $db, $driver, $table, $record, $params, $engine, $character_set) = @_; my $class = ref($proto) || $proto; @@ -103,6 +105,8 @@ sub new { character_set => $character_set, }; bless $self, $class; + weaken $self->{db}; + return $self; } diff --git a/lib/WeBWorK/DB/Schema/NewSQL/Merge.pm b/lib/WeBWorK/DB/Schema/NewSQL/Merge.pm index 7631e82fef..368cd4f14d 100644 --- a/lib/WeBWorK/DB/Schema/NewSQL/Merge.pm +++ b/lib/WeBWorK/DB/Schema/NewSQL/Merge.pm @@ -28,6 +28,8 @@ use warnings; use Carp qw(croak); use Iterator; use Iterator::Util; +use Scalar::Util qw(weaken); + use WeBWorK::DB::Utils::SQLAbstractIdentTrans; use WeBWorK::Debug; @@ -121,6 +123,7 @@ sub merge_init { sub sql_init { my $self = shift; + weaken $self; # Transformation function for table names. This allows us to pass the WeBWorK table names to # SQL::Abstract, and have it translate them to the SQL table names from tableOverride. diff --git a/lib/WeBWorK/DB/Schema/NewSQL/Std.pm b/lib/WeBWorK/DB/Schema/NewSQL/Std.pm index 794713200c..7869eed447 100644 --- a/lib/WeBWorK/DB/Schema/NewSQL/Std.pm +++ b/lib/WeBWorK/DB/Schema/NewSQL/Std.pm @@ -29,6 +29,8 @@ use Iterator; use Iterator::Util; use File::Temp; use String::ShellQuote; +use Scalar::Util qw(weaken); + use WeBWorK::DB::Utils::SQLAbstractIdentTrans; use WeBWorK::Debug; @@ -64,6 +66,7 @@ sub new { sub sql_init { my $self = shift; + weaken $self; # Transformation function for table names. This allows us to pass the WeBWorK table names to # SQL::Abstract, and have it translate them to the SQL table names from tableOverride. From ba749a449cb9719d4b556d59acd2aa5a959fa12d Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 17 Apr 2025 18:17:42 -0500 Subject: [PATCH 2/2] Instead of weakening the controller in the around_action hook, ensure that the $SIG{__WARN__} handler is reset in the after_dispatch hook so that the reference to the controller is released. --- lib/Mojolicious/WeBWorK.pm | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/Mojolicious/WeBWorK.pm b/lib/Mojolicious/WeBWorK.pm index 8bbbf8bd24..3a881fee2a 100644 --- a/lib/Mojolicious/WeBWorK.pm +++ b/lib/Mojolicious/WeBWorK.pm @@ -24,8 +24,7 @@ Mojolicious::WeBWorK - Mojolicious app for WeBWorK 2. use Env qw(WEBWORK_SERVER_ADMIN); -use Mojo::JSON qw(encode_json); -use Scalar::Util qw(weaken); +use Mojo::JSON qw(encode_json); use WeBWorK; use WeBWorK::CourseEnvironment; @@ -147,8 +146,6 @@ sub startup ($app) { around_action => async sub ($next, $c, $action, $last) { return $next->() unless $c->isa('WeBWorK::ContentGenerator'); - weaken $c; - my $uri = $c->req->url->path->to_string; $c->stash->{warnings} //= ''; @@ -174,7 +171,7 @@ sub startup ($app) { $app->hook( after_dispatch => sub ($c) { - $SIG{__WARN__} = $c->stash->{orig_sig_warn} if defined $c->stash->{orig_sig_warn}; + $SIG{__WARN__} = ref($c->stash->{orig_sig_warn}) eq 'CODE' ? $c->stash->{orig_sig_warn} : 'DEFAULT'; if ($c->isa('WeBWorK::ContentGenerator') && $c->ce) { $c->authen->store_session if $c->authen;