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.

Sign up for GitHub

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

Jump to bottom

NetworkHelper Singleton isn't thread safe #3197

Closed

michael-hawker opened this issue Mar 26, 2020 · 15 comments · Fixed by #3490

Labels

bug 🐛 Completed 🔥 improvements ✨

Milestone

Member

michael-hawker commented Mar 26, 2020 •

edited

Loading

Describe the bug

Noticed while working on unit tests that NetworkHelper isn't thread safe as we're assigning to a field instead of just using the property backing. We should update to the same pattern we used in #3168. I think in this case though, it's debatable whether it'd be a breaking change, probably best to put in 7.0 to be safe.

We should probably do an Audit in general of any Singleton patterns within the Toolkit, it seems like there may be other cases? See #3134 for more details.

michael-hawker added bug 🐛 improvements ✨ labels

michael-hawker added this to the 7.0 milestone

ghost added the needs triage 🔍 label

ghost commented Mar 26, 2020

Hello michael-hawker, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

ghost added the needs attention 👋 label

ghost commented Apr 10, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

10 similar comments

ghost commented Apr 25, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented May 11, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented May 26, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Jun 10, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Jun 25, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Jul 10, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Jul 25, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Aug 9, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Aug 24, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Sep 8, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

Sergio0694 mentioned this issue

Fixed thread safety for NetworkHelper.Instance #3490

Merged

7 tasks

ghost added the In-PR 🚀 label

ghost commented Oct 6, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

2 similar comments

ghost commented Oct 21, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

ghost commented Nov 6, 2020

This issue has been marked as "needs attention 👋" due to no activity for 15 days. Please triage the issue so the fix can be established.

michael-hawker removed needs attention 👋 needs triage 🔍 labels

ghost closed this as completed in #3490

ghost pushed a commit that referenced this issue


          Fixed thread safety for NetworkHelper.Instance (#3490)

43693f8

## Fixes #3197
<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

- Bugfix
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
The `NetworkHelper.Instance` property is not thread-safe.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
The `NetworkHelper.Instance` property is now thread safe and initialized in the implicit static constructor.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~
- [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~
    - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [ ] Contains **NO** breaking changes

ghost added Completed 🔥 and removed In-PR 🚀 labels

ghost locked as resolved and limited conversation to collaborators

This issue was closed.

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