Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
470443005:
Cronet modifications to support AGSA. (Closed)
|
Created:
6 years, 4 months ago by mdumitrescu Modified:
6 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, miloslav, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Project:
chromium Visibility:
Public. |
DescriptionExpose streaming upload support using chunked encoding.
ChromiumUrlRequestFactory now builds ChromiumUrlRequests rather than HttpUrlRequests.
BUG=390267
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290244
Patch Set 1 #
Total comments: 23
Patch Set 2 : Cronet modifications to support AGSA. #
Total comments: 13
Patch Set 3 : Small changes. #
Total comments: 2
Patch Set 4 : No more ugly threading. #Patch Set 5 : Check change. #
Total comments: 11
Patch Set 6 : Resync. #
Messages
Total messages: 16 (0 generated)
Looks pretty good. My question is whether you need to bring WritableByteChannel for those as well. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:30: nit: not needed nl? https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:156: public void appendChunkBlocking(ByteBuffer chunk, boolean isLastChunk) Due to limitations of our codereview tool we keep line length to 80 in order to avoid spurious wrapping. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:179: Thread.currentThread().interrupt(); How does that work? Where would interrupt come from? https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:184: throw mSinkException; Um, so where all those exceptions are getting caught? https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:498: private native void nativeAppendChunk(long urlRequestAdapter, ByteBuffer chunk, nit: limit line length to 80. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:525: nit: spurious nl? https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (left): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:110: url_request_->set_upload(upload_data_stream_.Pass()); Pass() is fine here. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:116: bool is_last_chunk) { nit: align with 'const'. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.h:16: #include "net/base/upload_data_stream.h" not needed? https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.h:109: bool GetFullRequestHeaders(net::HttpRequestHeaders* headers) const; This is already there (see 'ChromiumUrlRequest.getAllHeaders').
I've also removed a possible deadlock and added a couple CHECKs for invalid state/inputs. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:30: On 2014/08/15 13:30:22, mef wrote: > nit: not needed nl? Done. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:156: public void appendChunkBlocking(ByteBuffer chunk, boolean isLastChunk) On 2014/08/15 13:30:22, mef wrote: > Due to limitations of our codereview tool we keep line length to 80 in order to > avoid spurious wrapping. Done. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:179: Thread.currentThread().interrupt(); Changed to using condition variable. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:184: throw mSinkException; Changed to using condition variable. (The exceptions were caught on our side.) https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:498: private native void nativeAppendChunk(long urlRequestAdapter, ByteBuffer chunk, On 2014/08/15 13:30:22, mef wrote: > nit: limit line length to 80. Done. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ja... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:525: On 2014/08/15 13:30:22, mef wrote: > nit: spurious nl? Done. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (left): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:110: url_request_->set_upload(upload_data_stream_.Pass()); On 2014/08/15 13:30:22, mef wrote: > Pass() is fine here. Oops. Unintended change. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:116: bool is_last_chunk) { On 2014/08/15 13:30:22, mef wrote: > nit: align with 'const'. Done. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.h:16: #include "net/base/upload_data_stream.h" On 2014/08/15 13:30:22, mef wrote: > not needed? Done. https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.h:109: bool GetFullRequestHeaders(net::HttpRequestHeaders* headers) const; On 2014/08/15 13:30:22, mef wrote: > This is already there (see 'ChromiumUrlRequest.getAllHeaders'). That returns the response headers. We need the request headers as well.
https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:147: public void setChunkedUpload(String contentType) { setUploadChannel uses chunked uploads, too, so should probably rename this. https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:167: // to notify completion. This should be merged with the above comment, I think...At least I've never see two comment styles mixed like this. https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:194: mAppendChunkCondition.block(); We're blocking on another thread to avoid a string copy? Does this really make sense? I suppose these are rather large chunks? https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:114: CHECK(url_request_ != NULL); Not needed. We'd crash on the next line if this isn't true, anyways.
https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:99: bool URLRequestAdapter::GetFullRequestHeaders( Do you need this on the Java side? https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.h:109: bool GetFullRequestHeaders(net::HttpRequestHeaders* headers) const; On 2014/08/15 15:23:13, mdumitrescu wrote: > On 2014/08/15 13:30:22, mef wrote: > > This is already there (see 'ChromiumUrlRequest.getAllHeaders'). > > That returns the response headers. We need the request headers as well. Could you elaborate? What's your scenario and what are you planning to do with those? Specifically, when are you going to call this method? https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:147: public void setChunkedUpload(String contentType) { On 2014/08/15 15:30:21, mmenke wrote: > setUploadChannel uses chunked uploads, too, so should probably rename this. Actually, I think (correct me if I'm wrong) setUploadChannel does not use chunked uploads any more. That was a change upstreamed back in July, which has removed chunked uploads. https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:194: mAppendChunkCondition.block(); On 2014/08/15 15:30:21, mmenke wrote: > We're blocking on another thread to avoid a string copy? Does this really make > sense? I suppose these are rather large chunks? I think the idea is that one thread is writing into buffer and another (network thread) is uploading from buffer, hence the lock. nativeAppendChunk posts to network thread.
https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:147: public void setChunkedUpload(String contentType) { On 2014/08/15 15:42:52, mef wrote: > On 2014/08/15 15:30:21, mmenke wrote: > > setUploadChannel uses chunked uploads, too, so should probably rename this. > > Actually, I think (correct me if I'm wrong) setUploadChannel does not use > chunked uploads any more. That was a change upstreamed back in July, which has > removed chunked uploads. You're right, was forgetting it took a length. https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:194: mAppendChunkCondition.block(); On 2014/08/15 15:42:52, mef wrote: > On 2014/08/15 15:30:21, mmenke wrote: > > We're blocking on another thread to avoid a string copy? Does this really > make > > sense? I suppose these are rather large chunks? > I think the idea is that one thread is writing into buffer and another (network > thread) is uploading from buffer, hence the lock. nativeAppendChunk posts to > network thread. Right...but the question is if two copies is worse than blocking on whatever else the network thread is doing and a copy. In general, I am very much against blocking on another thread.
https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:99: bool URLRequestAdapter::GetFullRequestHeaders( On 2014/08/15 15:42:52, mef wrote: > Do you need this on the Java side? I was looking into this. It seems we don't right now so I might remove this (or add it to the java api). https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:167: // to notify completion. On 2014/08/15 15:30:21, mmenke wrote: > This should be merged with the above comment, I think...At least I've never see > two comment styles mixed like this. Well, the implementation details shouldn't be in the javadoc (unless relevant). Removed since it's pretty clear from the doc. https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:194: mAppendChunkCondition.block(); On 2014/08/15 15:30:21, mmenke wrote: > We're blocking on another thread to avoid a string copy? Does this really make > sense? I suppose these are rather large chunks? I'm very open to suggestions regarding this. A copy wouldn't necessarily be a problem (well, unless we'd have to allocateDirect). In my mind, the problem comes from url_request_adapter.cc. Specifically: void URLRequestAdapter::AppendChunk(const char* bytes, int bytes_len, bool is_last_chunk) { VLOG(1) << "AppendChunk, len: " << bytes_len << ", last: " << is_last_chunk; context_->GetNetworkTaskRunner()->PostTask( FROM_HERE, base::Bind(&URLRequestAdapter::OnAppendChunk, base::Unretained(this), bytes, bytes_len, is_last_chunk)); } Since this sends the data to another thread what happens if instead we return immediately, then call another appendChunk? Could the data become reordered? Or what happens if the cancel() or getHttpStatusCode() is called? Are these cases safe? If so, awesome, I'm all for making a copy right before context_->GetNetworkTaskRunner()->PostTask() and returning immediately. https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:114: CHECK(url_request_ != NULL); On 2014/08/15 15:30:21, mmenke wrote: > Not needed. We'd crash on the next line if this isn't true, anyways. Done.
https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:194: mAppendChunkCondition.block(); On 2014/08/15 15:50:20, mdumitrescu wrote: > On 2014/08/15 15:30:21, mmenke wrote: > > We're blocking on another thread to avoid a string copy? Does this really > make > > sense? I suppose these are rather large chunks? > > I'm very open to suggestions regarding this. > > A copy wouldn't necessarily be a problem (well, unless we'd have to > allocateDirect). In my mind, the problem comes from url_request_adapter.cc. > Specifically: > > void URLRequestAdapter::AppendChunk(const char* bytes, int bytes_len, > bool is_last_chunk) { > VLOG(1) << "AppendChunk, len: " << bytes_len << ", last: " << is_last_chunk; > context_->GetNetworkTaskRunner()->PostTask( > FROM_HERE, > base::Bind(&URLRequestAdapter::OnAppendChunk, > base::Unretained(this), > bytes, > bytes_len, > is_last_chunk)); > } > > Since this sends the data to another thread what happens if instead we return > immediately, then call another appendChunk? Could the data become reordered? Or > what happens if the cancel() or getHttpStatusCode() is called? Are these cases > safe? If so, awesome, I'm all for making a copy right before > context_->GetNetworkTaskRunner()->PostTask() and returning immediately. Tasks are executed in the order they're posted, since the NetworkTaskRunner is a SingleThreadedTaskRunner - a lot of things would break if that changed. cancel() is also safe. The upload data stream already has to handle both the case where it receives data and the network request isn't ready to read it, and where it's being used on cancellation. getHttpStatusCode is also safe. The one issue is that we need to make sure the buffer we're posting over to another thread isn't destroyed in the meantime. Simplest way to do that is to copy it. https://codereview.chromium.org/470443005/diff/40001/components/cronet/androi... File components/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/470443005/diff/40001/components/cronet/androi... components/cronet/android/org_chromium_net_UrlRequest.cc:225: CHECK(chunk_byte_buffer != NULL); Again, I don't think we need all these CHECKs. There's a bias in Chrome against them, since they have a non-zero cost in production builds. We'll certainly crash anyways in the first case, so all the CHECK does is bloat up the binary. I'm not sure if we'll crash in the second case, though if we switch to copying here, we well.
https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (right): https://codereview.chromium.org/470443005/diff/20001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:194: mAppendChunkCondition.block(); On 2014/08/15 16:09:24, mmenke wrote: > On 2014/08/15 15:50:20, mdumitrescu wrote: > > On 2014/08/15 15:30:21, mmenke wrote: > > > We're blocking on another thread to avoid a string copy? Does this really > > make > > > sense? I suppose these are rather large chunks? > > > > I'm very open to suggestions regarding this. > > > > A copy wouldn't necessarily be a problem (well, unless we'd have to > > allocateDirect). In my mind, the problem comes from url_request_adapter.cc. > > Specifically: > > > > void URLRequestAdapter::AppendChunk(const char* bytes, int bytes_len, > > bool is_last_chunk) { > > VLOG(1) << "AppendChunk, len: " << bytes_len << ", last: " << is_last_chunk; > > context_->GetNetworkTaskRunner()->PostTask( > > FROM_HERE, > > base::Bind(&URLRequestAdapter::OnAppendChunk, > > base::Unretained(this), > > bytes, > > bytes_len, > > is_last_chunk)); > > } > > > > Since this sends the data to another thread what happens if instead we return > > immediately, then call another appendChunk? Could the data become reordered? > Or > > what happens if the cancel() or getHttpStatusCode() is called? Are these cases > > safe? If so, awesome, I'm all for making a copy right before > > context_->GetNetworkTaskRunner()->PostTask() and returning immediately. > > Tasks are executed in the order they're posted, since the NetworkTaskRunner is a > SingleThreadedTaskRunner - a lot of things would break if that changed. > > cancel() is also safe. The upload data stream already has to handle both the > case where it receives data and the network request isn't ready to read it, and > where it's being used on cancellation. > > getHttpStatusCode is also safe. > > The one issue is that we need to make sure the buffer we're posting over to > another thread isn't destroyed in the meantime. Simplest way to do that is to > copy it. Done. https://codereview.chromium.org/470443005/diff/40001/components/cronet/androi... File components/cronet/android/org_chromium_net_UrlRequest.cc (right): https://codereview.chromium.org/470443005/diff/40001/components/cronet/androi... components/cronet/android/org_chromium_net_UrlRequest.cc:225: CHECK(chunk_byte_buffer != NULL); On 2014/08/15 16:09:24, mmenke wrote: > Again, I don't think we need all these CHECKs. There's a bias in Chrome against > them, since they have a non-zero cost in production builds. We'll certainly > crash anyways in the first case, so all the CHECK does is bloat up the binary. > I'm not sure if we'll crash in the second case, though if we switch to copying > here, we well. Done.
LGTM https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:72: VLOG(1) << "AppendChunk, len: " << bytes_len << ", last: " << is_last_chunk; include base/logging.h https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:74: memcpy(buf.get(), bytes, bytes_len); include string.h for memcpy. Should go below "url_request_adapter.h", with a blank line above and below. https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:76: FROM_HERE, include base/location.h https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:77: base::Bind(&URLRequestAdapter::OnAppendChunk, include base/bind.h
You will have to rebase it to latest as UrlRequest is now merged into ChromiumUrlRequest. Also, if have to do any callbacks from native, then you'll have to catch all exceptions and call onCalledByNativeException. https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (left): https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:42: private WritableByteChannel mOutputChannel; what happened to mOutputChannel?
https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (left): https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:42: private WritableByteChannel mOutputChannel; On 2014/08/15 19:42:27, mef wrote: > what happened to mOutputChannel? See mSink?
Resynced. How do I submit? https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (left): https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/UrlRequest.java:42: private WritableByteChannel mOutputChannel; On 2014/08/15 19:54:52, mmenke wrote: > On 2014/08/15 19:42:27, mef wrote: > > what happened to mOutputChannel? > > See mSink? Yeah. mOutputChannel is never used - mSink is. https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:72: VLOG(1) << "AppendChunk, len: " << bytes_len << ", last: " << is_last_chunk; On 2014/08/15 18:50:39, mmenke wrote: > include base/logging.h Done. https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:74: memcpy(buf.get(), bytes, bytes_len); On 2014/08/15 18:50:39, mmenke wrote: > include string.h for memcpy. Should go below "url_request_adapter.h", with a > blank line above and below. Done. https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:76: FROM_HERE, On 2014/08/15 18:50:39, mmenke wrote: > include base/location.h Done. https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:77: base::Bind(&URLRequestAdapter::OnAppendChunk, On 2014/08/15 18:50:39, mmenke wrote: > include base/bind.h Done.
On 2014/08/15 19:42:27, mef wrote: > You will have to rebase it to latest as UrlRequest is now merged into > ChromiumUrlRequest. Also, if have to do any callbacks from native, then you'll > have to catch all exceptions and call onCalledByNativeException. > > https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... > File components/cronet/android/java/src/org/chromium/net/UrlRequest.java (left): > > https://codereview.chromium.org/470443005/diff/80001/components/cronet/androi... > components/cronet/android/java/src/org/chromium/net/UrlRequest.java:42: private > WritableByteChannel mOutputChannel; > what happened to mOutputChannel? Thankfully, this isn't doin any callbacks from native anymore :).
The CQ bit was checked by mdumitrescu@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mdumitrescu@google.com/470443005/100001
Message was sent while issue was closed.
Committed patchset #6 (100001) as 290244 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
