Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845753002:
Remove data race in WebDataRequest::CancelRequest. (Closed)
|
Created:
3 years, 8 months ago by Roger McFarlane (Chromium) Modified:
3 years, 7 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, eroman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove data race in WebDataRequest::CancelRequest.
As an optimization, WebDataRequest::IsActive() was making an
(intentionally) unguarded nullptr check on its manager_ pointer
member in order to avoid doing some work if the request was
cancelled (which would set the pointer to nullptr in an
otherwise thread safe manner).
TSAN (correctly) detected and reported this race.
Given the latitude that an optimizing compiler is allowed in
rewriting code, it's not clear that this can be proven to be
a safe optimization... so, used atomic operations to protect
accesses to the manager_ member.
Additional related changes in this CL:
* Make the other members of WebDataRequest constants.
* Remove some thread safety debug checks that were validating
the assumptions made by the optimization (which was removed).
BUG=714789
Review-Url: https://codereview.chromium.org/2845753002
Cr-Commit-Position: refs/heads/master@{#469821}
Committed: https://chromium.googlesource.com/chromium/src/+/11cfe2438623a20f5ba74be863b6719b7525be18
Patch Set 1 #Patch Set 2 : minor tweaks #Patch Set 3 : BindOnce #Patch Set 4 : use atomic ops #
Total comments: 15
Patch Set 5 : pkasting's comments #Patch Set 6 : move static_assert #
Total comments: 2
Patch Set 7 : rebase #
Dependent Patchsets: Messages
Total messages: 39 (29 generated)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove data race in WebDataRequest::CancelRequest. As an optimization, WebDataRequest::IsActive() was making an (intentionally) unguarded nullptr check on its manager_ pointer member in order to avoid doing some work if the request was cancelled (which would set the pointer to nullptr in an otherwise thread safe manner). TSAN (correctly) detected and reported this race. Given the latitude that an optimizing compiler is allowed in rewriting code, it's not clear that this can be proven to be a safe optimization... so, adding a lock to protect accesses to the manager_ member. Additional related changes in this CL: * Make the other members of WebDataRequest constants. * Remove some thread safety debug checks that were validating the assumptions made by the optimization (which was removed). BUG=714789 ========== to ========== Remove data race in WebDataRequest::CancelRequest. As an optimization, WebDataRequest::IsActive() was making an (intentionally) unguarded nullptr check on its manager_ pointer member in order to avoid doing some work if the request was cancelled (which would set the pointer to nullptr in an otherwise thread safe manner). TSAN (correctly) detected and reported this race. Given the latitude that an optimizing compiler is allowed in rewriting code, it's not clear that this can be proven to be a safe optimization... so, used atomic operations to protect accesses to the manager_ member. Additional related changes in this CL: * Make the other members of WebDataRequest constants. * Remove some thread safety debug checks that were validating the assumptions made by the optimization (which was removed). BUG=714789 ==========
The CQ bit was unchecked by rogerm@chromium.org
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
rogerm@chromium.org changed reviewers: + pkasting@chromium.org
PTAL? https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:138: DCHECK(i != pending_requests_.end()); The code in this block is just CancelRequest() + a ScoperTracker. This could equivalently by: void WebDataRequestManager::RequestCompletedOnThread( std::unique_ptr<WebDataRequest> request, std::unique_ptr<WDTypedResult> result) { if (!request->IsActive()) return; { // This tracker could be moved into CancelRequest? tracked_objects::ScopedTracker tracking_profile( FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); CancelRequest(request->GetHandle()); } auto* const consumer = request->GetConsumer(); if (consumer) { tracked_objects::ScopedTracker tracking_profile( FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); consumer->OnWebDataServiceRequestDone(request->GetHandle()); } }
https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (left): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:146: consumer = request->GetConsumer(); This isn't safe to remove. MarkAsInactive() nukes the |consumer_| on the request, which you'll need afterwards. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:79: new WebDataRequest(this, consumer, next_request_handle_)); Nit: Prefer = MakeUnique to bare new https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:124: // another thread before the lock was acquired. This comment seems misleading (we're no longer "re-"checking anything, since the first check was removed). https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:138: DCHECK(i != pending_requests_.end()); On 2017/04/27 20:51:44, Roger McFarlane (Chromium) wrote: > The code in this block is just CancelRequest() + a ScoperTracker. > > This could equivalently by: > > void WebDataRequestManager::RequestCompletedOnThread( > std::unique_ptr<WebDataRequest> request, > std::unique_ptr<WDTypedResult> result) { > if (!request->IsActive()) > return; > > { > // This tracker could be moved into CancelRequest? > tracked_objects::ScopedTracker tracking_profile( > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > CancelRequest(request->GetHandle()); > } > > auto* const consumer = request->GetConsumer(); > if (consumer) { > tracked_objects::ScopedTracker tracking_profile( > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > consumer->OnWebDataServiceRequestDone(request->GetHandle()); > } > } Doing this will require some refactoring given the bit about needing to save off the consumer. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.h (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.h:58: WebDataRequestManager* GetManager() const; Const methods should not return non-const pointers. Either make the pointer const or the method non-const.
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. Another look? https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (left): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:146: consumer = request->GetConsumer(); On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > This isn't safe to remove. MarkAsInactive() nukes the |consumer_| on the > request, which you'll need afterwards. consumer_ doesn't gets nuked anymore (I made it const). https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:79: new WebDataRequest(this, consumer, next_request_handle_)); On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > Nit: Prefer = MakeUnique to bare new That doesn't play nicely with non-public constructors. I wasn't able to find any occurrences of friend base::MakeUnique. Is it better to make the constructor public? https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:124: // another thread before the lock was acquired. On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > This comment seems misleading (we're no longer "re-"checking anything, since the > first check was removed). Done. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:138: DCHECK(i != pending_requests_.end()); On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > On 2017/04/27 20:51:44, Roger McFarlane (Chromium) wrote: > > The code in this block is just CancelRequest() + a ScoperTracker. > > > > This could equivalently by: > > > > void WebDataRequestManager::RequestCompletedOnThread( > > std::unique_ptr<WebDataRequest> request, > > std::unique_ptr<WDTypedResult> result) { > > if (!request->IsActive()) > > return; > > > > { > > // This tracker could be moved into CancelRequest? > > tracked_objects::ScopedTracker tracking_profile( > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > CancelRequest(request->GetHandle()); > > } > > > > auto* const consumer = request->GetConsumer(); > > if (consumer) { > > tracked_objects::ScopedTracker tracking_profile( > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > consumer->OnWebDataServiceRequestDone(request->GetHandle()); > > } > > } > > Doing this will require some refactoring given the bit about needing to save off > the consumer. consumer_ no longer gets set to nullptr, only the active/inactive state is toggled by MarkAsInactive. one could also grab the consumer before hand. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.h (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.h:58: WebDataRequestManager* GetManager() const; On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > Const methods should not return non-const pointers. Either make the pointer > const or the method non-const. Removed const qualifiers as needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/28 15:06:13, Roger McFarlane (Chromium) wrote: > Thanks. Another look? > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > File components/webdata/common/web_data_request_manager.cc (left): > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > components/webdata/common/web_data_request_manager.cc:146: consumer = > request->GetConsumer(); > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > This isn't safe to remove. MarkAsInactive() nukes the |consumer_| on the > > request, which you'll need afterwards. > > consumer_ doesn't gets nuked anymore (I made it const). > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > File components/webdata/common/web_data_request_manager.cc (right): > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > components/webdata/common/web_data_request_manager.cc:79: new > WebDataRequest(this, consumer, next_request_handle_)); > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > Nit: Prefer = MakeUnique to bare new > > That doesn't play nicely with non-public constructors. I wasn't able to find any > occurrences of friend base::MakeUnique. Is it better to make the constructor > public? > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > components/webdata/common/web_data_request_manager.cc:124: // another thread > before the lock was acquired. > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > This comment seems misleading (we're no longer "re-"checking anything, since > the > > first check was removed). > > Done. > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > components/webdata/common/web_data_request_manager.cc:138: DCHECK(i != > pending_requests_.end()); > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > On 2017/04/27 20:51:44, Roger McFarlane (Chromium) wrote: > > > The code in this block is just CancelRequest() + a ScoperTracker. > > > > > > This could equivalently by: > > > > > > void WebDataRequestManager::RequestCompletedOnThread( > > > std::unique_ptr<WebDataRequest> request, > > > std::unique_ptr<WDTypedResult> result) { > > > if (!request->IsActive()) > > > return; > > > > > > { > > > // This tracker could be moved into CancelRequest? > > > tracked_objects::ScopedTracker tracking_profile( > > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > > CancelRequest(request->GetHandle()); > > > } > > > > > > auto* const consumer = request->GetConsumer(); > > > if (consumer) { > > > tracked_objects::ScopedTracker tracking_profile( > > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > > consumer->OnWebDataServiceRequestDone(request->GetHandle()); > > > } > > > } > > > > Doing this will require some refactoring given the bit about needing to save > off > > the consumer. > > consumer_ no longer gets set to nullptr, only the active/inactive state is > toggled by MarkAsInactive. > > one could also grab the consumer before hand. > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > File components/webdata/common/web_data_request_manager.h (right): > > https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... > components/webdata/common/web_data_request_manager.h:58: WebDataRequestManager* > GetManager() const; > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > Const methods should not return non-const pointers. Either make the pointer > > const or the method non-const. > > Removed const qualifiers as needed. ping?
LGTM (sorry, was OOO) https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:79: new WebDataRequest(this, consumer, next_request_handle_)); On 2017/04/28 15:06:13, Roger McFarlane (Chromium) wrote: > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > Nit: Prefer = MakeUnique to bare new > > That doesn't play nicely with non-public constructors. I wasn't able to find any > occurrences of friend base::MakeUnique. Is it better to make the constructor > public? Mmm. I didn't think through the constructor being non-public. Probably better to just leave it as you had it, then. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:138: DCHECK(i != pending_requests_.end()); On 2017/04/28 15:06:13, Roger McFarlane (Chromium) wrote: > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > On 2017/04/27 20:51:44, Roger McFarlane (Chromium) wrote: > > > The code in this block is just CancelRequest() + a ScoperTracker. > > > > > > This could equivalently by: > > > > > > void WebDataRequestManager::RequestCompletedOnThread( > > > std::unique_ptr<WebDataRequest> request, > > > std::unique_ptr<WDTypedResult> result) { > > > if (!request->IsActive()) > > > return; > > > > > > { > > > // This tracker could be moved into CancelRequest? > > > tracked_objects::ScopedTracker tracking_profile( > > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > > CancelRequest(request->GetHandle()); > > > } > > > > > > auto* const consumer = request->GetConsumer(); > > > if (consumer) { > > > tracked_objects::ScopedTracker tracking_profile( > > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > > consumer->OnWebDataServiceRequestDone(request->GetHandle()); > > > } > > > } > > > > Doing this will require some refactoring given the bit about needing to save > off > > the consumer. > > consumer_ no longer gets set to nullptr, only the active/inactive state is > toggled by MarkAsInactive. In that case, the change you suggest seems like a good one. Could happen in this CL or a separate one. https://codereview.chromium.org/2845753002/diff/140001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/140001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:32: bool WebDataRequest::IsActive() { Losing the "const" here seems sad, but inlining the complexity of GetManager() or adding a parallel, const version of that function also seem kinda sad. Maybe this is the easiest solution.
Thanks. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:79: new WebDataRequest(this, consumer, next_request_handle_)); On 2017/05/04 18:32:25, Peter Kasting (catching up) wrote: > On 2017/04/28 15:06:13, Roger McFarlane (Chromium) wrote: > > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > > Nit: Prefer = MakeUnique to bare new > > > > That doesn't play nicely with non-public constructors. I wasn't able to find > any > > occurrences of friend base::MakeUnique. Is it better to make the constructor > > public? > > Mmm. I didn't think through the constructor being non-public. Probably better > to just leave it as you had it, then. Acknowledged. https://codereview.chromium.org/2845753002/diff/100001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:138: DCHECK(i != pending_requests_.end()); On 2017/05/04 18:32:25, Peter Kasting (catching up) wrote: > On 2017/04/28 15:06:13, Roger McFarlane (Chromium) wrote: > > On 2017/04/28 06:36:41, Peter Kasting (OOO thru May 3) wrote: > > > On 2017/04/27 20:51:44, Roger McFarlane (Chromium) wrote: > > > > The code in this block is just CancelRequest() + a ScoperTracker. > > > > > > > > This could equivalently by: > > > > > > > > void WebDataRequestManager::RequestCompletedOnThread( > > > > std::unique_ptr<WebDataRequest> request, > > > > std::unique_ptr<WDTypedResult> result) { > > > > if (!request->IsActive()) > > > > return; > > > > > > > > { > > > > // This tracker could be moved into CancelRequest? > > > > tracked_objects::ScopedTracker tracking_profile( > > > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > > > CancelRequest(request->GetHandle()); > > > > } > > > > > > > > auto* const consumer = request->GetConsumer(); > > > > if (consumer) { > > > > tracked_objects::ScopedTracker tracking_profile( > > > > FROM_HERE_WITH_EXPLICIT_FUNCTION("...")); > > > > consumer->OnWebDataServiceRequestDone(request->GetHandle()); > > > > } > > > > } > > > > > > Doing this will require some refactoring given the bit about needing to save > > off > > > the consumer. > > > > consumer_ no longer gets set to nullptr, only the active/inactive state is > > toggled by MarkAsInactive. > > In that case, the change you suggest seems like a good one. Could happen in > this CL or a separate one. i'll send another CL. https://codereview.chromium.org/2845753002/diff/140001/components/webdata/com... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2845753002/diff/140001/components/webdata/com... components/webdata/common/web_data_request_manager.cc:32: bool WebDataRequest::IsActive() { On 2017/05/04 18:32:25, Peter Kasting (catching up) wrote: > Losing the "const" here seems sad, but inlining the complexity of GetManager() > or adding a parallel, const version of that function also seem kinda sad. Maybe > this is the easiest solution. Acknowledged.
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2845753002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rogerm@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494021785715680,
"parent_rev": "04f35409eaca0779dbe9b8435f20901cf510440b", "commit_rev":
"11cfe2438623a20f5ba74be863b6719b7525be18"}
Message was sent while issue was closed.
Description was changed from ========== Remove data race in WebDataRequest::CancelRequest. As an optimization, WebDataRequest::IsActive() was making an (intentionally) unguarded nullptr check on its manager_ pointer member in order to avoid doing some work if the request was cancelled (which would set the pointer to nullptr in an otherwise thread safe manner). TSAN (correctly) detected and reported this race. Given the latitude that an optimizing compiler is allowed in rewriting code, it's not clear that this can be proven to be a safe optimization... so, used atomic operations to protect accesses to the manager_ member. Additional related changes in this CL: * Make the other members of WebDataRequest constants. * Remove some thread safety debug checks that were validating the assumptions made by the optimization (which was removed). BUG=714789 ========== to ========== Remove data race in WebDataRequest::CancelRequest. As an optimization, WebDataRequest::IsActive() was making an (intentionally) unguarded nullptr check on its manager_ pointer member in order to avoid doing some work if the request was cancelled (which would set the pointer to nullptr in an otherwise thread safe manner). TSAN (correctly) detected and reported this race. Given the latitude that an optimizing compiler is allowed in rewriting code, it's not clear that this can be proven to be a safe optimization... so, used atomic operations to protect accesses to the manager_ member. Additional related changes in this CL: * Make the other members of WebDataRequest constants. * Remove some thread safety debug checks that were validating the assumptions made by the optimization (which was removed). BUG=714789 Review-Url: https://codereview.chromium.org/2845753002 Cr-Commit-Position: refs/heads/master@{#469821} Committed: https://chromium.googlesource.com/chromium/src/+/11cfe2438623a20f5ba74be863b6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/11cfe2438623a20f5ba74be863b6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
