Skip to content

Conversation

@lamxdoan
Copy link
Contributor

@lamxdoan lamxdoan commented Feb 25, 2020

Fix #4174

  • Add locking around modifications to m_requests map.
Microsoft Reviewers: Open in CodeFlow

@lamxdoan lamxdoan requested a review from a team as a code owner February 25, 2020 19:20
@ghost ghost added the vnext label Feb 25, 2020
@msftclas
Copy link

msftclas commented Feb 25, 2020

CLA assistant check
All CLA requirements met.

@vmoroz
Copy link
Member

vmoroz commented Feb 25, 2020

auto iter = m_requests.find(requestId);

Should we use mutex here as well? #Resolved


Refers to: vnext/ReactUWP/Modules/NetworkingModule.cpp:342 in 80e32c9. [](commit_id = 80e32c9, deletion_comment = False)

@lamxdoan
Copy link
Contributor Author

auto iter = m_requests.find(requestId);

Yeah I think it makes sense to add it here as well.


In reply to: 591128047 [](ancestors = 591128047)


Refers to: vnext/ReactUWP/Modules/NetworkingModule.cpp:342 in 80e32c9. [](commit_id = 80e32c9, deletion_comment = False)

@vmoroz
Copy link
Member

vmoroz commented Feb 26, 2020

It seems that your PR is missing a change file. Please run 'yarn change' to create it. #Resolved

Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

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

:shipit:

winrt::Windows::Web::Http::HttpResponseMessage,
winrt::Windows::Web::Http::HttpProgress> httpRequest(nullptr);
{
std::scoped_lock(m_mutex);
Copy link
Member

Choose a reason for hiding this comment

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

( [](start = 20, length = 1)

There must be a variable name for the lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good catch. Fixed.


In reply to: 384211714 [](ancestors = 384211714)

@vmoroz vmoroz merged commit 0072e5d into microsoft:master Feb 26, 2020
lamxdoan added a commit to lamxdoan/react-native-windows that referenced this pull request Feb 26, 2020
* Add locking around access to m_requests

* Change files

* add locking around AbortRequest as well

* fix formatting

* lock needs local variable
ghost pushed a commit that referenced this pull request Feb 26, 2020
* Fix concurrency issue in NetworkingModule (#4179)

* Add locking around access to m_requests

* Change files

* add locking around AbortRequest as well

* fix formatting

* lock needs local variable

* Remove useIncrementalUpdates assert from Networking module (#4116)

* Remove useIncrementalUpdates assert from Networking module

* Change files

* add [[maybe_unused]]

Co-authored-by: Marlene Cota <marlenecota@gmail.com>
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.

Concurrency issue in NetworkingModule causes _STL_VERIFY to fail

3 participants