Fix deletion of Array elements #172

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@ngsankha

Node-IRC uses delete keyword to remove things from Array which actually sets it to undefined. This causes unexpected errors like:

/home/ubuntu/scrollback/node_modules/irc/lib/irc.js:637
                    throw err;
                          ^
TypeError: Cannot call method 'replace' of undefined
    at /home/ubuntu/scrollback/node_modules/irc/lib/irc.js:239:71
    at Array.forEach (native)
    at Client.<anonymous> (/home/ubuntu/scrollback/node_modules/irc/lib/irc.js:226:26)
    at Client.EventEmitter.emit (events.js:95:17)
    at /home/ubuntu/scrollback/node_modules/irc/lib/irc.js:634:22
    at Array.forEach (native)
    at Socket.<anonymous> (/home/ubuntu/scrollback/node_modules/irc/lib/irc.js:631:15)
    at Socket.EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:720:14)
    at Socket.EventEmitter.emit (events.js:92:17)

This patch fixes this problem.

@jirwin

This comment has been minimized.

Show comment
Hide comment
@jirwin

jirwin Jun 18, 2013

Collaborator

I don't think you want to do this. Because these are objects, and not arrays, I'm lead to believe that you aren't using Object.hasOwnProperty() while iterating these. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty has a good explanation of how to use this to check for the existence of properties on an Object.

Collaborator

jirwin commented Jun 18, 2013

I don't think you want to do this. Because these are objects, and not arrays, I'm lead to believe that you aren't using Object.hasOwnProperty() while iterating these. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty has a good explanation of how to use this to check for the existence of properties on an Object.

@ngsankha

This comment has been minimized.

Show comment
Hide comment
@ngsankha

ngsankha Jun 18, 2013

Yes, if we don't want to do this, then I think we should just check for an undefined before calling a String.prototype.replace, to prevent that error from occurring.

I could send a pull request with it if that's required to be done.

Yes, if we don't want to do this, then I think we should just check for an undefined before calling a String.prototype.replace, to prevent that error from occurring.

I could send a pull request with it if that's required to be done.

@jirwin

This comment has been minimized.

Show comment
Hide comment
@jirwin

jirwin Jun 18, 2013

Collaborator

Maybe if you included what you were doing it would be easier to figure out the right thing to do. I'm assuming you are iterating one of these objects, else you would know if something existed or not. I would make my loop look something like:

for (var chan in channels) {
  if (channel.hasOwnProperty(chan)) {
    <do stuff>
  }
}
Collaborator

jirwin commented Jun 18, 2013

Maybe if you included what you were doing it would be easier to figure out the right thing to do. I'm assuming you are iterating one of these objects, else you would know if something existed or not. I would make my loop look something like:

for (var chan in channels) {
  if (channel.hasOwnProperty(chan)) {
    <do stuff>
  }
}
@damianb

This comment has been minimized.

Show comment
Hide comment
@damianb

damianb Jun 18, 2013

Contributor

I suspect this is the cause of #171, due to array delete mechanics not being a true delete; if we wanted a proper delete, then there's always the Array.prototype.remove extension that John Resig wrote.

http://stackoverflow.com/a/9815010/783103 && http://ejohn.org/blog/javascript-array-remove/

ed: Yeah, this is the problem. node-irc's builtin tracking of users-in-channel and such is the culprit of those crashes, and this would be the solution.

Also, @jirwin's concern completely misses the mark here imo. These are arrays, not objects, as delete's behavior is leaving behind undefined and not removing the object property itself.

Contributor

damianb commented Jun 18, 2013

I suspect this is the cause of #171, due to array delete mechanics not being a true delete; if we wanted a proper delete, then there's always the Array.prototype.remove extension that John Resig wrote.

http://stackoverflow.com/a/9815010/783103 && http://ejohn.org/blog/javascript-array-remove/

ed: Yeah, this is the problem. node-irc's builtin tracking of users-in-channel and such is the culprit of those crashes, and this would be the solution.

Also, @jirwin's concern completely misses the mark here imo. These are arrays, not objects, as delete's behavior is leaving behind undefined and not removing the object property itself.

@ngsankha

This comment has been minimized.

Show comment
Hide comment
@ngsankha

ngsankha Jun 18, 2013

Yes, its also because of the same reason. This pull request fixes this by using the similar solution as given in that Stack Overflow link.

Yes, its also because of the same reason. This pull request fixes this by using the similar solution as given in that Stack Overflow link.

@damianb

This comment has been minimized.

Show comment
Hide comment
@damianb

damianb Jun 18, 2013

Contributor

It may be worthwhile to write a tiny wrapper function around splice to indicate that it's for deleting an array entry if Resig's solution isn't used; using the splice as-is is a little harder to read and grok right away.

Contributor

damianb commented Jun 18, 2013

It may be worthwhile to write a tiny wrapper function around splice to indicate that it's for deleting an array entry if Resig's solution isn't used; using the splice as-is is a little harder to read and grok right away.

@jirwin

This comment has been minimized.

Show comment
Hide comment
@jirwin

jirwin Jun 18, 2013

Collaborator

@damianb Lets look at what objects this PR interacts with, and where those are defined in the code.

Unless I'm missing something completely, these are all Objects and not arrays. It looks like https://github.com/sankha93/node-irc/blob/35fe7e9426cadf893e5f05fe27a0d665bfe11d28/lib/irc.js#L276 is iterating self.chans incorrectly for how it is defined. I still think the right fix for this is to use hasOwnProperty() everywhere these objects are being iterated.

Collaborator

jirwin commented Jun 18, 2013

@damianb Lets look at what objects this PR interacts with, and where those are defined in the code.

Unless I'm missing something completely, these are all Objects and not arrays. It looks like https://github.com/sankha93/node-irc/blob/35fe7e9426cadf893e5f05fe27a0d665bfe11d28/lib/irc.js#L276 is iterating self.chans incorrectly for how it is defined. I still think the right fix for this is to use hasOwnProperty() everywhere these objects are being iterated.

@damianb

This comment has been minimized.

Show comment
Hide comment
@damianb

damianb Jun 18, 2013

Contributor

Just blindly using hasOwnProperty isn't a proper way to handle it though. If there's undefined just sitting inside the object/array/whathaveyou, that's allocated memory that should be freed back up.

Contributor

damianb commented Jun 18, 2013

Just blindly using hasOwnProperty isn't a proper way to handle it though. If there's undefined just sitting inside the object/array/whathaveyou, that's allocated memory that should be freed back up.

@jirwin

This comment has been minimized.

Show comment
Hide comment
@jirwin

jirwin Jun 18, 2013

Collaborator

@damianb I really don't believe that to be the case. Look at this snippet from the node REPL:

node
> var foo = {baz: 'qux'};
undefined
> foo
{ baz: 'qux' }
> foo['baz'];
'qux'
> foo['monkey'];
undefined
> delete foo['baz'];
true
> foo['baz'];
undefined
> foo
{}

Javascript returns undefined for any property that doesn't exist on the object.

Collaborator

jirwin commented Jun 18, 2013

@damianb I really don't believe that to be the case. Look at this snippet from the node REPL:

node
> var foo = {baz: 'qux'};
undefined
> foo
{ baz: 'qux' }
> foo['baz'];
'qux'
> foo['monkey'];
undefined
> delete foo['baz'];
true
> foo['baz'];
undefined
> foo
{}

Javascript returns undefined for any property that doesn't exist on the object.

@damianb

This comment has been minimized.

Show comment
Hide comment
@damianb

damianb Jun 18, 2013

Contributor
> var obj = { entry: 'something' }
undefined
> obj.entry = undefined
undefined
> obj
{ entry: undefined }
> var obj = { entry: 'something' }
undefined
> delete obj['entry']
true
> obj
{}

There is a reason that property is still being iterated over; check Object.keys, it's probably still listed. And if it's listed, memory is still allocated.

Contributor

damianb commented Jun 18, 2013

> var obj = { entry: 'something' }
undefined
> obj.entry = undefined
undefined
> obj
{ entry: undefined }
> var obj = { entry: 'something' }
undefined
> delete obj['entry']
true
> obj
{}

There is a reason that property is still being iterated over; check Object.keys, it's probably still listed. And if it's listed, memory is still allocated.

@jirwin

This comment has been minimized.

Show comment
Hide comment
@jirwin

jirwin Jun 18, 2013

Collaborator

@damianb There is a difference between an Object property with an undefined value, and a property that isn't defined on the Object. To use your own example:

node
> var foo = {baz: 'qux'};
undefined
> foo
{ baz: 'qux' }
> foo['baz'];
'qux'
> Object.keys(foo);
[ 'baz' ]
> foo['monkey'];
undefined
> delete foo['baz'];
true
> foo['baz'];
undefined
> foo
{}
> Object.keys(foo);
[]
Collaborator

jirwin commented Jun 18, 2013

@damianb There is a difference between an Object property with an undefined value, and a property that isn't defined on the Object. To use your own example:

node
> var foo = {baz: 'qux'};
undefined
> foo
{ baz: 'qux' }
> foo['baz'];
'qux'
> Object.keys(foo);
[ 'baz' ]
> foo['monkey'];
undefined
> delete foo['baz'];
true
> foo['baz'];
undefined
> foo
{}
> Object.keys(foo);
[]
@damianb

This comment has been minimized.

Show comment
Hide comment
@damianb

damianb Jun 18, 2013

Contributor

...Yes, and the problem here that is causing crashes is _an entry or property that has an undefined value_ that the code is attempting to iterate on and interact with.

Contributor

damianb commented Jun 18, 2013

...Yes, and the problem here that is causing crashes is _an entry or property that has an undefined value_ that the code is attempting to iterate on and interact with.

@ngsankha

This comment has been minimized.

Show comment
Hide comment
@ngsankha

ngsankha Jun 20, 2013

So did we reach any unanimous decision over here? I would still like to avoid delete and then checking for hasOwnProperty because in that case we are iterating over the property anyways.

So did we reach any unanimous decision over here? I would still like to avoid delete and then checking for hasOwnProperty because in that case we are iterating over the property anyways.

@martynsmith

This comment has been minimized.

Show comment
Hide comment
@martynsmith

martynsmith Sep 6, 2013

Owner

Okay, after reading all the comments, I'm inclined to think this pull request is the wrong solution.

Clearly the references in question are objects rather than arrays. Sure, on https://github.com/sankha93/node-irc/blob/35fe7e9426cadf893e5f05fe27a0d665bfe11d28/lib/irc.js#L276 the iteration should probably be protected with a hasOwnProperty call (or simpler still, use underscore).

I'm going to close this pull request.

If someone wants to make a new one that fixes the loop above, I'd happily merge that :-)

Owner

martynsmith commented Sep 6, 2013

Okay, after reading all the comments, I'm inclined to think this pull request is the wrong solution.

Clearly the references in question are objects rather than arrays. Sure, on https://github.com/sankha93/node-irc/blob/35fe7e9426cadf893e5f05fe27a0d665bfe11d28/lib/irc.js#L276 the iteration should probably be protected with a hasOwnProperty call (or simpler still, use underscore).

I'm going to close this pull request.

If someone wants to make a new one that fixes the loop above, I'd happily merge that :-)

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