Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784143003:
Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established (Closed)
DescriptionFix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established
When a new SpdySession is created, we iterate through
|spdy_session_request_map_| and notify all pending requests
(HttpStreamFactoryImpl::OnNewSpdySessionReady).
We will enter an infinite loop here because the iteration doesn't
guarantee to update |spdy_session_request_map_| container, contrary to
what the comments say.
The Request destructor removes itself from |spdy_session_request_map_|.
However, if the owner of Request doesn't destroy the Request in
OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction
always destroys the Request in OnStreamReady()
callback, so we do not run into this situation in practice.
I think the RemoveRequestFromSpdySessionRequestMap() should
be directly invoked by the FactoryImpl, instead of indirectly
through Request destructor and the Controller.
This CL adds a regression test which will trigger the infinite
loop in release build.
BUG=706974
Patch Set 1 : self #Patch Set 2 : rebased #Patch Set 3 : rebased #
Total comments: 12
Patch Set 4 : fix one more test #Patch Set 5 : Address comments #
Messages
Total messages: 38 (34 generated)
Description was changed from ========== Fix HttpStreamFactoryImpl to not complete Request twice BUG=706974 ========== to ========== Fix an infinite loop in HttpStreamFactoryImpl when new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. This CL inlines HttpStreamFactoryImpl::OnNewSpdySessionReady, so we can use the RemoveRequestFromSpdySessionRequestMap() logic. BUG=706974 ==========
Description was changed from ========== Fix an infinite loop in HttpStreamFactoryImpl when new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. This CL inlines HttpStreamFactoryImpl::OnNewSpdySessionReady, so we can use the RemoveRequestFromSpdySessionRequestMap() logic. BUG=706974 ========== to ========== Fix an infinite loop in HttpStreamFactoryImpl when new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. This CL inlines HttpStreamFactoryImpl::OnNewSpdySessionReady, so we can use the RemoveRequestFromSpdySessionRequestMap() logic. BUG=706974 ==========
xunjieli@chromium.org changed reviewers: + bnc@chromium.org, mmenke@chromium.org
xunjieli@chromium.org changed reviewers: - bnc@chromium.org, mmenke@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by xunjieli@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...
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Fix an infinite loop in HttpStreamFactoryImpl when new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. This CL inlines HttpStreamFactoryImpl::OnNewSpdySessionReady, so we can use the RemoveRequestFromSpdySessionRequestMap() logic. BUG=706974 ========== to ========== Fix an infinite loop in HttpStreamFactoryImpl when new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. This CL adds a regression test which will trigger the infinite loop in release build (i.e with DCHECK removed). BUG=706974 ==========
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Fix an infinite loop in HttpStreamFactoryImpl when new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. This CL adds a regression test which will trigger the infinite loop in release build (i.e with DCHECK removed). BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirect invocation through the destructor. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
Description was changed from ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirect invocation through the destructor. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirect invocation through the destructor. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
Description was changed from ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirect invocation through the destructor. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through the destructor. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
Description was changed from ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through the destructor. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
xunjieli@chromium.org changed reviewers: + bnc@chromium.org, mmenke@chromium.org
This is a bit ugly. Let me know if you have any suggestion.
The CQ bit was checked by xunjieli@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration never updates the container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
Description was changed from ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests. We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests (HttpStreamFactoryImpl::OnNewSpdySessionReady). We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Description was changed from ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests (HttpStreamFactoryImpl::OnNewSpdySessionReady). We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of by indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests (HttpStreamFactoryImpl::OnNewSpdySessionReady). We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
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 ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests (HttpStreamFactoryImpl::OnNewSpdySessionReady). We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. However, I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ========== to ========== Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established When a new SpdySession is created, we iterate through |spdy_session_request_map_| and notify all pending requests (HttpStreamFactoryImpl::OnNewSpdySessionReady). We will enter an infinite loop here because the iteration doesn't guarantee to update |spdy_session_request_map_| container, contrary to what the comments say. The Request destructor removes itself from |spdy_session_request_map_|. However, if the owner of Request doesn't destroy the Request in OnStreamReady() callback, we will be in trouble. HttpNetworkTransaction always destroys the Request in OnStreamReady() callback, so we do not run into this situation in practice. I think the RemoveRequestFromSpdySessionRequestMap() should be directly invoked by the FactoryImpl, instead of indirectly through Request destructor and the Controller. This CL adds a regression test which will trigger the infinite loop in release build. BUG=706974 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:253: if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key)) Once you are here, consider removing one of these two lookups here as well by using find() directly. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:422: if (!base::ContainsKey(spdy_session_request_map_, *spdy_session_key)) Please use |iterator it = find(*spdy_session_key); if (iterator == end()) return; request_set = it.second;| or something similar to avoid doing the lookup twice. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:425: if (base::ContainsKey(request_set, request)) Skip the ContainsKey() call in order to avoid doing the lookup twice: it is valid to call erase() even if the key does not exist, in which case it is a no-op. Maybe add a comment if you think it is necessary. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:428: spdy_session_request_map_.erase(*spdy_session_key); And use the iterator here to avoid the third lookup. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:628: if (!request_->HasSpdySessionKey()) { Is it possible that |request_| already has a SpdySessionKey at this point? And if it is, will a pointer to |request_| live forever under the old, hereby overwritten key, in |factory_->spdy_session_request_map_|? If it's not possible, maybe remove HasSpdySessionKey() altogether, and add a DCHECK(!spdy_session_key_) within SetSpdySessionKey(). https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:2156: // Request two streams at once and make sure they use the same connection. It's not obvious to me where you make sure that they use the same connection.
The CQ bit was checked by xunjieli@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...
https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:253: if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key)) On 2017/03/31 16:21:27, Bence wrote: > Once you are here, consider removing one of these two lookups here as well by > using find() directly. Done. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:422: if (!base::ContainsKey(spdy_session_request_map_, *spdy_session_key)) On 2017/03/31 16:21:27, Bence wrote: > Please use |iterator it = find(*spdy_session_key); if (iterator == end()) > return; request_set = it.second;| or something similar to avoid doing the lookup > twice. Done. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:425: if (base::ContainsKey(request_set, request)) On 2017/03/31 16:21:27, Bence wrote: > Skip the ContainsKey() call in order to avoid doing the lookup twice: it is > valid to call erase() even if the key does not exist, in which case it is a > no-op. Maybe add a comment if you think it is necessary. Done. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl.cc:428: spdy_session_request_map_.erase(*spdy_session_key); On 2017/03/31 16:21:27, Bence wrote: > And use the iterator here to avoid the third lookup. Done. Good idea! https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl_job_controller.cc:628: if (!request_->HasSpdySessionKey()) { On 2017/03/31 16:21:27, Bence wrote: > Is it possible that |request_| already has a SpdySessionKey at this point? And > if it is, will a pointer to |request_| live forever under the old, hereby > overwritten key, in |factory_->spdy_session_request_map_|? > > If it's not possible, maybe remove HasSpdySessionKey() altogether, and add a > DCHECK(!spdy_session_key_) within SetSpdySessionKey(). Good catch! It's possible that |request| already has a SpdySessionKey. The Job can be restarted (e.g. for proxy auth). In that case, this method will be called again with the same key. I don't see a path where this is called with a different key though. I added a check to remove the old key.Done. https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... File net/http/http_stream_factory_impl_unittest.cc (right): https://codereview.chromium.org/2784143003/diff/140001/net/http/http_stream_f... net/http/http_stream_factory_impl_unittest.cc:2156: // Request two streams at once and make sure they use the same connection. On 2017/03/31 16:21:27, Bence wrote: > It's not obvious to me where you make sure that they use the same connection. I asserted on the number of spdy session established EXPECT_EQ(1, GetSpdySessionCount(session.get())); Will that be enough?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Abandoning this CL. This is much harder than I thought. In particular, if we have also have an alt Spdy job pointing to a different host/port, we will be calling SetSpdySessionKey() with two different keys on the same Impl::Request. The second SetSpdySessonKey() is an no-op, so if the second job succeeds, we will fail to notify pending Requests. This will probably be a problem when H2 connection pooling is implemented/enabled. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
