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 memory leak in waitDevice method #75

Merged
merged 3 commits into from
Nov 10, 2024

Conversation

Raffone17
Copy link
Contributor

In the waitDevice method, if the device is not found the 'operation timed out' error is thrown but the interval is not cleared. I added a try and finally statements for avoid this problem.

@gmacario
Copy link

Hello @chrvadala,

Please let us know when you plan to have a look at this PR by @Raffone17. Thank you!

@chrvadala
Copy link
Owner

Thanks for this PR guys, can you integrate it with a test?
I see that the fix makes completely sense, but it is better to cover this case with a test.
You can mock getDevice for that. It seems the easier solution

@Raffone17
Copy link
Contributor Author

Hello @chrvadala,
I did a unit test that covers the scenario.

Copy link
Owner

@chrvadala chrvadala left a comment

Choose a reason for hiding this comment

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

Thanks for adding a test. I added few comments inline.

const discoveryInterval = 100

const spyClearInterval = jest.spyOn(global, 'clearInterval')
const spyClearTimeout = jest.spyOn(global, 'clearInterval')
Copy link
Owner

Choose a reason for hiding this comment

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

I think that there's a mistake on this line.

@@ -142,4 +142,31 @@ describe('waitDevice', () => {

return res
})

test('clear intervals and timeouts after fail', async () => {
jest.useFakeTimers('legacy')
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a specific reason for using the legacy implementation?


const waitDevicePromise = adapter.waitDevice('44:44:44:44:44:44', timeout, discoveryInterval)

jest.advanceTimersByTime(500)
Copy link
Owner

Choose a reason for hiding this comment

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

you can specify here the timeout variable that you declared

Suggested change
jest.advanceTimersByTime(500)
jest.advanceTimersByTime(timeout)

@Raffone17
Copy link
Contributor Author

@chrvadala I made the requested changes.

@chrvadala chrvadala merged commit 79213f0 into chrvadala:main Nov 10, 2024
6 checks passed
@chrvadala
Copy link
Owner

Released with v1.12.0
Thanks @Raffone17 and @gmacario for your help on this project

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.

3 participants