Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move _callbacks to an object to avoid empty slots #172

Merged
merged 1 commit into from Sep 23, 2021

Conversation

humaidk2
Copy link
Contributor

I noticed by logging sessions that _callbacks kept growing to [null,null,null,null...] for every smpp request
After investigation, I found the issue to be that the use of an array for _callbacks.
This pr changes it to an object to solve the issue.

Fixes #170

@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.62% when pulling 8253d77 on humaidk2:fix-callbacks-issue into fe53dcc on farhadi:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 75.62% when pulling 8253d77 on humaidk2:fix-callbacks-issue into fe53dcc on farhadi:master.

@juliangut juliangut added this to the 0.6.0 milestone Sep 22, 2021
@juliangut
Copy link
Collaborator

@humaidk2 I'm ready to prepare next release ASAP and I'd like to add this if needed

I've tested your PR branch and the project keeps working all right

But I haven't been able to reproduce the problem you mention, I've tried this simple test directly in node 6, 8, 10, 12, and 14 and those nulls you mention

var _callbacks = [];
_callbacks['one'] = function(){};
_callbacks['two'] = function(){};
_callbacks['three'] = function(){};
_callbacks['four'] = function(){};
console.log(_callbacks);
delete _callbacks['two'];
console.log(_callbacks);

var _callbacks = {};
_callbacks['one'] = function(){};
_callbacks['two'] = function(){};
_callbacks['three'] = function(){};
_callbacks['four'] = function(){};
console.log(_callbacks);
delete _callbacks['two'];
console.log(_callbacks);
delete _callbacks.three;
console.log(_callbacks);

Can you tell me which node version are you using?

@juliangut juliangut modified the milestones: 0.6.0, 0.5.1 Sep 23, 2021
@humaidk2
Copy link
Contributor Author

humaidk2 commented Sep 23, 2021

Hay Julian,

your example works fine, but try this out

var _callbacks = [];
_callbacks[1] = function(){};
_callbacks[2] = function(){};
_callbacks[3] = function(){};
_callbacks[4] = function(){};
console.log(_callbacks);
delete _callbacks[2];
console.log(_callbacks);

var _callbacks = {};
_callbacks[1] = function(){};
_callbacks[2] = function(){};
_callbacks[3] = function(){};
_callbacks[4] = function(){};
console.log(_callbacks);
delete _callbacks[2];
console.log(_callbacks);

Based on the code, it's definitely using numbers as the sequence number, not strings
Screen Shot 2021-09-23 at 2 03 25 PM

@juliangut
Copy link
Collaborator

Great catch indeed

It's even worse if sequence_number does not start at 0 but at a higher number the array will have plenty of null values right from the start

Thanks for noticing and creating this PR, will merge

@juliangut juliangut merged commit 1d54cee into farhadi:master Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_callbacks should use an object instead of an array
3 participants