Skip to content

Commit f3c27bf

Browse files
committed
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.
1 parent 2d4cbcc commit f3c27bf

File tree

8 files changed

+15
-42
lines changed

8 files changed

+15
-42
lines changed

lib/Mojolicious/WeBWorK.pm

+4-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ Mojolicious::WeBWorK - Mojolicious app for WeBWorK 2.
2424

2525
use Env qw(WEBWORK_SERVER_ADMIN);
2626

27-
use Mojo::JSON qw(encode_json);
27+
use Mojo::JSON qw(encode_json);
28+
use Scalar::Util qw(weaken);
2829

2930
use WeBWorK;
3031
use WeBWorK::CourseEnvironment;
@@ -146,6 +147,8 @@ sub startup ($app) {
146147
around_action => async sub ($next, $c, $action, $last) {
147148
return $next->() unless $c->isa('WeBWorK::ContentGenerator');
148149

150+
weaken $c;
151+
149152
my $uri = $c->req->url->path->to_string;
150153
$c->stash->{warnings} //= '';
151154

lib/WeBWorK/Authen/LTIAdvanced.pm

-23
Original file line numberDiff line numberDiff line change
@@ -40,29 +40,6 @@ use WeBWorK::Authen::LTIAdvanced::Nonce;
4040

4141
$Net::OAuth::PROTOCOL_VERSION = Net::OAuth::PROTOCOL_VERSION_1_0A;
4242

43-
=head1 CONSTRUCTOR
44-
45-
=over
46-
47-
=item new($c)
48-
49-
Instantiates a new WeBWorK::Authen object for the given WeBWorK::Controller ($c).
50-
51-
=cut
52-
53-
sub new {
54-
my ($invocant, $c) = @_;
55-
my $class = ref($invocant) || $invocant;
56-
my $self = { c => $c, };
57-
#initialize
58-
bless $self, $class;
59-
return $self;
60-
}
61-
62-
=back
63-
64-
=cut
65-
6643
## this is only overridden for debug logging
6744
#sub verify {
6845
# debug("BEGIN LTIAdvanced VERIFY");

lib/WeBWorK/Authen/LTIAdvantage.pm

-16
Original file line numberDiff line numberDiff line change
@@ -33,22 +33,6 @@ use WeBWorK::Utils::DateTime qw(formatDateTime);
3333
use WeBWorK::Utils::Instructor qw(assignSetToUser);
3434
use WeBWorK::Authen::LTIAdvantage::SubmitGrade;
3535

36-
=head1 CONSTRUCTOR
37-
38-
=over
39-
40-
=item new($c)
41-
42-
Instantiates a new WeBWorK::Authen object for the given WeBWorK::Controller ($c).
43-
44-
=back
45-
46-
=cut
47-
48-
sub new ($invocant, $c) {
49-
return bless { c => $c }, ref($invocant) || $invocant;
50-
}
51-
5236
sub request_has_data_for_this_verification_module ($self) {
5337
debug('LTIAdvantage has been called for data verification');
5438
my $c = $self->{c};

lib/WeBWorK/Authz.pm

+1-1
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ sub new {
8484
my ($invocant, $c) = @_;
8585
my $class = ref($invocant) || $invocant;
8686
my $self = { c => $c, };
87-
#weaken $self->{c};
87+
weaken $self->{c};
8888

8989
$c->{permission_retrieval_error} = 0;
9090
bless $self, $class;

lib/WeBWorK/ContentGenerator/Home.pm

-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ WeBWorK::ContentGenerator::Home - display a list of courses.
2323
=cut
2424

2525
use WeBWorK::Utils::Files qw(readFile);
26-
use WeBWorK::Localize;
2726

2827
sub info ($c) {
2928
my $result;

lib/WeBWorK/DB/Schema.pm

+4
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ dependent. C<$db> is provided so that schemas can query other schemas.
8181
8282
=cut
8383

84+
use Scalar::Util qw(weaken);
85+
8486
sub new {
8587
my ($proto, $db, $driver, $table, $record, $params, $engine, $character_set) = @_;
8688
my $class = ref($proto) || $proto;
@@ -103,6 +105,8 @@ sub new {
103105
character_set => $character_set,
104106
};
105107
bless $self, $class;
108+
weaken $self->{db};
109+
106110
return $self;
107111
}
108112

lib/WeBWorK/DB/Schema/NewSQL/Merge.pm

+3
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ use warnings;
2828
use Carp qw(croak);
2929
use Iterator;
3030
use Iterator::Util;
31+
use Scalar::Util qw(weaken);
32+
3133
use WeBWorK::DB::Utils::SQLAbstractIdentTrans;
3234
use WeBWorK::Debug;
3335

@@ -121,6 +123,7 @@ sub merge_init {
121123

122124
sub sql_init {
123125
my $self = shift;
126+
weaken $self;
124127

125128
# Transformation function for table names. This allows us to pass the WeBWorK table names to
126129
# SQL::Abstract, and have it translate them to the SQL table names from tableOverride.

lib/WeBWorK/DB/Schema/NewSQL/Std.pm

+3
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ use Iterator;
2929
use Iterator::Util;
3030
use File::Temp;
3131
use String::ShellQuote;
32+
use Scalar::Util qw(weaken);
33+
3234
use WeBWorK::DB::Utils::SQLAbstractIdentTrans;
3335
use WeBWorK::Debug;
3436

@@ -64,6 +66,7 @@ sub new {
6466

6567
sub sql_init {
6668
my $self = shift;
69+
weaken $self;
6770

6871
# Transformation function for table names. This allows us to pass the WeBWorK table names to
6972
# SQL::Abstract, and have it translate them to the SQL table names from tableOverride.

0 commit comments

Comments
 (0)