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

fix: the API of es6-promisify is not the same as promisify-es6 #905

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Mar 26, 2021

Fixes a regression introduced by #896

promisify-es6 was swapped for es6-promisify but the APIs of the two modules are no the same so the NAT hole punching code broke and stopped libp2p nodes UDP ports open which prevented processes from exiting.

Also changes port numbers to 0 in the tests as other processes can be listening on those ports which causes spurious test failures.

Fixes a regression introduced by #896

`promisify-es6` was swapped for `es6-promisify` but the APIs of the
two modules are no the same so the NAT hole punching code broke.

Also changes port numbers to 0 in the tests as other processes can be
listening on those ports which causes spurious test failures.
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for NAT hole punching code broke and stopped libp2p nodes UDP ports open which prevented processes from exiting, so that we avoid future refressions

@achingbrain
Copy link
Member Author

The UDP ports are opened by the @motrix/nat-api library which is functioning correctly so I'm not sure that needs testing - the ports were left open because .destroy() was not being called on the client instance on shut down as the client creation was throwing due to the wrong API being used.

I've added a test that would have caught the error.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! LGTM

@vasco-santos vasco-santos merged commit a7128f0 into master Mar 26, 2021
@vasco-santos vasco-santos deleted the fix/promisify-nat-manager-methods branch March 26, 2021 17:13
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.

2 participants