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(windows): refactor implementation to avoid crashes #564

Merged
merged 5 commits into from Jan 25, 2022

Conversation

aschultz
Copy link
Contributor

@aschultz aschultz commented Jan 20, 2022

Overview

The existing Windows implementation shared state across multiple threads without proper synchronization and allowed asynchronous NetworkStatusChanged events to access invalid memory during teardown of the module. The shared state wasn't really necessary, so I've streamlined the code to avoid it altogether.

Manual serialization to JSValue has been replaced with the newer macro-based automation. I've also added support for a few netinfo properties that were missing on Windows, though they require the "wiFiControl" capability be specified in the appxmanifest.

Test Plan

Ethernet connection:
image
image

WiFi connection:
image

Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Seems like a great addition - I had one comment just related to documenting the new device capability

I'll try to pull it and build it locally just to verify but in general LGTM pending doc

@mikehardy
Copy link
Contributor

Just a note that checking this out locally and building it (with commit from #565 picked locally to run things) - everything seems to work for me. I have no wifi on my test windows VM and it just returns null for those entries FWIW

Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@mikehardy mikehardy added the pending merge A PR that will be merged shortly, waiting for CI or final comment label Jan 25, 2022
@mikehardy mikehardy changed the title Refactor Windows implementation to avoid crashes fix(windows): refactor implementation to avoid crashes Jan 25, 2022
@mikehardy mikehardy removed the pending merge A PR that will be merged shortly, waiting for CI or final comment label Jan 25, 2022
@mikehardy mikehardy merged commit cc4bfa3 into react-native-netinfo:master Jan 25, 2022
github-actions bot pushed a commit that referenced this pull request Jan 25, 2022
## [7.1.8](v7.1.7...v7.1.8) (2022-01-25)

### Bug Fixes

* **windows:** refactor implementation to avoid crashes ([#564](#564)) ([cc4bfa3](cc4bfa3))
@matt-oakes
Copy link
Collaborator

🎉 This PR is included in version 7.1.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants