Stop leaking "smart" variable #1051

Merged
merged 13 commits into from Nov 21, 2013

Conversation

3 participants
@elliotf
Contributor

elliotf commented Nov 11, 2013

Hello. I'm not sure why this isn't showing up in the sequelize test suite, but it looks like 'smart' gets leaked, and this should fix it.

@janmeier

This comment has been minimized.

Show comment
Hide comment
@janmeier

janmeier Nov 11, 2013

Member

Good catch! Could you please move the var declaration to the top of the functions?

 HasManySingleLinked.prototype.injectGetter = function(options) {
    var self = this
      , where = {}
      , smart

etc...

Do you have any suggestions for a test suite that could prevent global variable leaks :) ?

Member

janmeier commented Nov 11, 2013

Good catch! Could you please move the var declaration to the top of the functions?

 HasManySingleLinked.prototype.injectGetter = function(options) {
    var self = this
      , where = {}
      , smart

etc...

Do you have any suggestions for a test suite that could prevent global variable leaks :) ?

@durango

This comment has been minimized.

Show comment
Hide comment
@durango

durango Nov 11, 2013

Member

@janmeier JSHint should automatically help you out with that, idk why my editor didn't catch this when I was typing though... odd.

Member

durango commented Nov 11, 2013

@janmeier JSHint should automatically help you out with that, idk why my editor didn't catch this when I was typing though... odd.

@janmeier

This comment has been minimized.

Show comment
Hide comment
@janmeier

janmeier Nov 11, 2013

Member

Sounds weird @durango, perhaps you have accidentally overridden your setting for undef :) ?

Member

janmeier commented Nov 11, 2013

Sounds weird @durango, perhaps you have accidentally overridden your setting for undef :) ?

@durango

This comment has been minimized.

Show comment
Hide comment
@durango

durango Nov 11, 2013

Member

No I was trying out Sublime around this time, I think the plugin I was using was kind of wonky (made for 2 but was using version 3). Vim should be better :p

Member

durango commented Nov 11, 2013

No I was trying out Sublime around this time, I think the plugin I was using was kind of wonky (made for 2 but was using version 3). Vim should be better :p

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 11, 2013

Contributor

I'm using mocha, just like sequelize is. That's why I'm confused that this doesn't show up in the normal run.

I grep'ed through the code base for settings to disable global leak detection and didn't find anything. After putting some console log()s in, the test suite is definitely running the code, but mocha isn't complaining.

Contributor

elliotf commented Nov 11, 2013

I'm using mocha, just like sequelize is. That's why I'm confused that this doesn't show up in the normal run.

I grep'ed through the code base for settings to disable global leak detection and didn't find anything. After putting some console log()s in, the test suite is definitely running the code, but mocha isn't complaining.

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 11, 2013

Contributor

Well, I managed to force mocha to check for global leaks by adding the --check-leaks command line param to the Makefile. I thought that was the default, but... I don't know. At least it complains now.

Should I add that change to the Makefile to this pull request (along with other global fixes) or should I start a separate pull request?

Contributor

elliotf commented Nov 11, 2013

Well, I managed to force mocha to check for global leaks by adding the --check-leaks command line param to the Makefile. I thought that was the default, but... I don't know. At least it complains now.

Should I add that change to the Makefile to this pull request (along with other global fixes) or should I start a separate pull request?

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 11, 2013

Contributor

I'll go ahead and add them to this pull request and if you want something different, I can change it.

Contributor

elliotf commented Nov 11, 2013

I'll go ahead and add them to this pull request and if you want something different, I can change it.

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 11, 2013

Contributor

The last leak that occurs is 'columnTypes' in lib/dialects/sqlite/query.js :

this.columnTypes = columnTypes;

I have to say, this code made me look twice. At first I was confused, as "this" is being referred to inside an anonymous function, but I can only assume that a call() or apply() is being called somewhere?

The problem is that under an error condition, "this" is the global variable.

Contributor

elliotf commented Nov 11, 2013

The last leak that occurs is 'columnTypes' in lib/dialects/sqlite/query.js :

this.columnTypes = columnTypes;

I have to say, this code made me look twice. At first I was confused, as "this" is being referred to inside an anonymous function, but I can only assume that a call() or apply() is being called somewhere?

The problem is that under an error condition, "this" is the global variable.

@janmeier

This comment has been minimized.

Show comment
Hide comment
@janmeier

janmeier Nov 12, 2013

Member

That code is a bit confusing yes, and I am not quite sure what it actually does :). I would think that for a success the second argument to the callback would be column types. Could you test if

  err ? onFailure.call(self, err) : onSuccess.call(self, results, { columnTypes: columnType})

Works?

Member

janmeier commented Nov 12, 2013

That code is a bit confusing yes, and I am not quite sure what it actually does :). I would think that for a success the second argument to the callback would be column types. Could you test if

  err ? onFailure.call(self, err) : onSuccess.call(self, results, { columnTypes: columnType})

Works?

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 12, 2013

Contributor

"this" in that block is some sort of sql object:

{ sql: 'SELECT name FROM sqlite_master WHERE type=\'table\' and name!=\'sqlite_sequence\';',
  columnTypes: {} }

If I don't set columnTypes on 'this' and instead pass just the columnTypes then things break.

Also, the global leak only occurs if there is an error, which is interesting. The specific test that triggers the leak is:

  1) [SQLITE] DAO complete gets triggered if an error occurs:

It seems to me that the problem is not the code block itself, but instead whatever is calling it or setting the 'this' context. I was trying to figure out where the call/apply that is called inside:

        self.database[getDatabaseMethod.call(self)](self.sql, function(err, results) {

From this line:

self.database[getDatabaseMethod.call(self)](self.sql, function(err, results) {

? The code is a little hard to read, but if I could find out what 'self.database' is on that line, that would help. It seems to be the thing that is calling apply/call and setting the 'this' context.

Contributor

elliotf commented Nov 12, 2013

"this" in that block is some sort of sql object:

{ sql: 'SELECT name FROM sqlite_master WHERE type=\'table\' and name!=\'sqlite_sequence\';',
  columnTypes: {} }

If I don't set columnTypes on 'this' and instead pass just the columnTypes then things break.

Also, the global leak only occurs if there is an error, which is interesting. The specific test that triggers the leak is:

  1) [SQLITE] DAO complete gets triggered if an error occurs:

It seems to me that the problem is not the code block itself, but instead whatever is calling it or setting the 'this' context. I was trying to figure out where the call/apply that is called inside:

        self.database[getDatabaseMethod.call(self)](self.sql, function(err, results) {

From this line:

self.database[getDatabaseMethod.call(self)](self.sql, function(err, results) {

? The code is a little hard to read, but if I could find out what 'self.database' is on that line, that would help. It seems to be the thing that is calling apply/call and setting the 'this' context.

@janmeier

This comment has been minimized.

Show comment
Hide comment
@janmeier

janmeier Nov 19, 2013

Member

self.database refers to query.database, which is set in the constructor. Query is constructed in connector manager, where you can see that database is a reference to an sqlite3 instance

this.database = db = new sqlite3.Database(self.sequelize.options.storage || ':memory:', function(err) {

Hope this helps, and please give a heads up when this PR is ready to merge. Thanks for helping to make the code more awesome! :)

Also, feel free to change the makefile to include the flags to mocha so we always check for variable leaks

Member

janmeier commented Nov 19, 2013

self.database refers to query.database, which is set in the constructor. Query is constructed in connector manager, where you can see that database is a reference to an sqlite3 instance

this.database = db = new sqlite3.Database(self.sequelize.options.storage || ':memory:', function(err) {

Hope this helps, and please give a heads up when this PR is ready to merge. Thanks for helping to make the code more awesome! :)

Also, feel free to change the makefile to include the flags to mocha so we always check for variable leaks

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 19, 2013

Contributor

Okay, that code almost makes sense now. Thank you very much!

Contributor

elliotf commented Nov 19, 2013

Okay, that code almost makes sense now. Thank you very much!

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 20, 2013

Contributor

After reading through the code and the sqlite3 documentation, it seems that the "this" context inside the block starting on line 35 is the statement object: https://github.com/mapbox/node-sqlite3/wiki/API#databaserunsql-param--callback

It seems a little odd to be annotating another librarie's object with column types, though.

Two things:

  • I took the liberty of removing some semicolons to keep in line with the recommended style.
  • I changed functionality slightly by not setting column types on the context object in an error condition. Tests seem to pass, and it would have been on the global object previously, so I think it's fine.
Contributor

elliotf commented Nov 20, 2013

After reading through the code and the sqlite3 documentation, it seems that the "this" context inside the block starting on line 35 is the statement object: https://github.com/mapbox/node-sqlite3/wiki/API#databaserunsql-param--callback

It seems a little odd to be annotating another librarie's object with column types, though.

Two things:

  • I took the liberty of removing some semicolons to keep in line with the recommended style.
  • I changed functionality slightly by not setting column types on the context object in an error condition. Tests seem to pass, and it would have been on the global object previously, so I think it's fine.
@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 20, 2013

Contributor

Ah, crud. It looks like there are probably globals leaking in other sql engines (mysql, postgres) ... Let me get the other DBs up and running so I can find the leaks.

Contributor

elliotf commented Nov 20, 2013

Ah, crud. It looks like there are probably globals leaking in other sql engines (mysql, postgres) ... Let me get the other DBs up and running so I can find the leaks.

@elliotf

This comment has been minimized.

Show comment
Hide comment
@elliotf

elliotf Nov 20, 2013

Contributor

Okay, we are a good for pull request merge.

Contributor

elliotf commented Nov 20, 2013

Okay, we are a good for pull request merge.

janmeier added a commit that referenced this pull request Nov 21, 2013

@janmeier janmeier merged commit 53007f2 into sequelize:master Nov 21, 2013

1 check passed

default The Travis CI build passed
Details
@janmeier

This comment has been minimized.

Show comment
Hide comment
@janmeier

janmeier Nov 21, 2013

Member

Awesome, thanks! :)

Member

janmeier commented Nov 21, 2013

Awesome, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment