Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1440193002:
Fix DTLS packet boundary handling in SSLStreamAdapterTests. (Closed)
DescriptionFix DTLS packet boundary handling in SSLStreamAdapterTests.
The tests were not honoring packet boundaries, thus causing failures
in tests with dropped/broken packets. This CL fixes this and also
re-enables the tests.
R=torbjorng@webrtc.org,pthatcher@webrtc.org,tommi@webrtc.org,juberti@webrtc.org
BUG=webrtc:5005, webrtc:5188
Committed: https://crrev.com/e488a0dbe4114ce51feeaf663ad4e2a6bd4b9a2b
Cr-Commit-Position: refs/heads/master@{#10709}
Patch Set 1 #
Total comments: 23
Patch Set 2 : First changes based on feedback from tommi #
Total comments: 26
Patch Set 3 : Feedback from torbjorng + BufferQueue now implements StreamInterface #Patch Set 4 : Small cleanup #
Total comments: 5
Patch Set 5 : No longer inherit BufferQueue from StreamInterface #
Total comments: 2
Messages
Total messages: 26 (5 generated)
Ptal
Great work :) I think it would be good if we could use rtc::Bind for the callback instead of sigslot and always use the callback on the same thread. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.cc File webrtc/base/bufferqueue.cc (right): https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.cc#ne... webrtc/base/bufferqueue.cc:101: void BufferQueue::OnMessage(Message* msg) { It looks like we allow posting this notification to any thread. Is that intentional? If it's a requirement of an external implementation to receive the callback on a different thread than the main thread, I would prefer to have that detail handled in the implementation that depends on it. If we remove that capability from BufferQueue, we greatly simplify the threading model. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... webrtc/base/bufferqueue.h:33: int events; for consistency keep members below methods https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... webrtc/base/bufferqueue.h:40: MSG_POST_EVENT = 0x1 private? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... webrtc/base/bufferqueue.h:63: sigslot::signal2<BufferQueue*, int> SignalEvent; can we use rtc::Bind instead? If not, can we make this member variable private? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... webrtc/base/bufferqueue.h:68: void PostEvent(Thread* t, int events); nit: t -> thread Should this be private? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... webrtc/base/bufferqueue.h:79: Thread* owner_; if the value of this never changes, lets make it Thread* const owner_; What about calling it owning_thread_, main_thread_ or ctor_thread_ instead of owner_? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:81: explicit SSLDummyStreamBase(SSLStreamAdapterTestBase *test, no need for explicit. can you change it so that the location of * and & comes directly after the type? (here and below) https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:90: // Defined later nit: Can you expand on this comment a bit? Is there a default implementation of this method but not the other write? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:98: SSLStreamAdapterTestBase *test_; SSLStreamAdapterTestBase* test_; you could also name it test_base_; https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:202: << " events=" << events << " forwarding upward"; fix indent (4 spaces or preferably align the <<) https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:211: << " events=" << events << " forwarding upward"; and here https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:300: client_ssl_.reset(NULL); nit: use nullptr. Is this necessary though? Won't this happen automatically when the test is deleted? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:426: size_t data_len, size_t *written, can you fix the indentation since you're here? https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:727: }; remove semicolon
Thanks for your feedback, updated the tests. Regarding the comments on the BufferQueue: I want to inherit that from StreamInterface in another CL to automatically get the event signalling functionality I added here, but thought that these changes are out of scope for this CL. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:81: explicit SSLDummyStreamBase(SSLStreamAdapterTestBase *test, On 2015/11/13 08:38:13, tommi (webrtc) wrote: > no need for explicit. > can you change it so that the location of * and & comes directly after the type? > > (here and below) Done. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:90: // Defined later On 2015/11/13 08:38:13, tommi (webrtc) wrote: > nit: Can you expand on this comment a bit? Is there a default implementation of > this method but not the other write? This comment was there before, I don't think it makes much sense anyway - the implementation is further down below which is perfectly normal. Removed the comment. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:98: SSLStreamAdapterTestBase *test_; On 2015/11/13 08:38:12, tommi (webrtc) wrote: > SSLStreamAdapterTestBase* test_; > > you could also name it test_base_; Done. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:202: << " events=" << events << " forwarding upward"; On 2015/11/13 08:38:13, tommi (webrtc) wrote: > fix indent (4 spaces or preferably align the <<) Done. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:211: << " events=" << events << " forwarding upward"; On 2015/11/13 08:38:13, tommi (webrtc) wrote: > and here Done. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:300: client_ssl_.reset(NULL); On 2015/11/13 08:38:13, tommi (webrtc) wrote: > nit: use nullptr. Is this necessary though? Won't this happen automatically > when the test is deleted? Changed to nullptr. This is necessary because otherwise the subclass will release the underlying FifoBuffer/BufferQueue in the dtor while it is still used in the dtor here. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:426: size_t data_len, size_t *written, On 2015/11/13 08:38:13, tommi (webrtc) wrote: > can you fix the indentation since you're here? Done here and in some other places. https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/sslstreamadapter_... webrtc/base/sslstreamadapter_unittest.cc:727: }; On 2015/11/13 08:38:13, tommi (webrtc) wrote: > remove semicolon Done.
Description was changed from ========== Fix DTLS packet boundary handling in SSLStreamAdapterTests. The tests were not honoring packet boundaries, thus causing failures in tests with dropped/broken packets. This CL fixes this and also re-enables the tests. R=torbjorng@webrtc.org,pthatcher@webrtc.org,tommi@webrtc.org,juberti@webrtc.org BUG=webrtc:5005,webrtc:5188 ========== to ========== Fix DTLS packet boundary handling in SSLStreamAdapterTests. The tests were not honoring packet boundaries, thus causing failures in tests with dropped/broken packets. This CL fixes this and also re-enables the tests. R=torbjorng@webrtc.org,pthatcher@webrtc.org,tommi@webrtc.org,juberti@webrtc.org BUG=webrtc:5005,webrtc:5188 ==========
guoweis@webrtc.org changed reviewers: + guoweis@webrtc.org
Great stuff! I have some comments, but nothing major; this looks very good. There are a few things we need to clean up related to buffer sizes. I commented on them here, since your CL adds another buffer size parameter. We can clean it up in a separate CL. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.cc File webrtc/base/bufferqueue.cc (right): https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... webrtc/base/bufferqueue.cc:21: : capacity_(capacity), default_size_(default_size), owner_(owner) { Consider combining the two constructors, by using a default argument, i.e., ..., Thread owner = Thread::Current(). (Default arguments are good for ctors.) https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... webrtc/base/bufferqueue.cc:50: size_t next_packet_size = packet->size(); Consider using bytes = std::min(packet->size(), bytes) for legibility. (I realise this is outside of current CL.) https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... webrtc/base/bufferqueue.cc:93: void BufferQueue::PostEvent(Thread* t, int events) { Nit: t => thread https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: // BufferEvents are used to asynchronously signal state transitionss. The flags Nit: "transitionss" is misspelled. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:39: enum { Nit: Define enum on one line for consistency. Please use "git cl format" to make these sort of things consistent. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:45: // Creates a buffer queue queue with a given capacity, default buffer size Nit: "queue queue" https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:54: // that don't fit in the passed memory. Please add "Returns true unless no data could be returned." or something with a similar meaning. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:67: // re-entrancy. I don't understand the 2nd sentence here. What does "unroll the stack" mean? By "prevent re-entrancy", do you mean that this makes sure non-reentrant code is not re-entered? (Which code?) https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:73: // MessageHandler Interface Nit: The code style asks for a full stop after every comment. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:82: mutable CriticalSection crit_; // object lock Consider using GUARDED_BY here. See e.g. webrtc/call/rtc_event_log.cc for an example if its use. Nit: The comment "object lock" seems unnecessary. If you think it is, please start with Capital letter and end with full stop. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:157: virtual rtc::StreamResult WriteData(const void* data, size_t data_len, Please omit "virtual" for override methods (here and in several places below). https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:228: LOG(LS_INFO) << "Closing outbound stream"; Nit: Not a stream. :-) https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:239: static const int kBufferCapacity = 1; I thought capacity >= size should hold for these buffers. What does it mean that kBufferCapacity = 1 here? Related to the buffer sizes is code in SSLStreamAdapterTestDTLS::ReadData and ::WriteData. We should lift out their sizes, now oddly and inconsistently hardwired to 1600 (allocation in WriteData), 1000 (actually hardwired but via packet_size_), and 2000 (hardwired in ReadData). The data handled by BufferQueue is encrypted and slightly expanded wrt non-encrypted data. The buffer sizes need to account for that. (I am aware that these hardwired sizes were there prior to your changes.)
Ok, this got larger than I wanted it to be ;-) Anyways, BufferQueue now implements StreamInterface which made the test code a lot cleaner (actually wanted to do that in a separate CL). Also made more changes based on feedback from torbjorng. Ptal https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... webrtc/base/bufferqueue.h:79: Thread* owner_; On 2015/11/13 08:38:12, tommi (webrtc) wrote: > if the value of this never changes, lets make it Thread* const owner_; > What about calling it owning_thread_, main_thread_ or ctor_thread_ instead of > owner_? Renamed, but can't change to "const" ("PostEvent" needs a non-const Thread). https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.cc File webrtc/base/bufferqueue.cc (right): https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... webrtc/base/bufferqueue.cc:21: : capacity_(capacity), default_size_(default_size), owner_(owner) { On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Consider combining the two constructors, by using a default argument, i.e., ..., > Thread owner = Thread::Current(). > > (Default arguments are good for ctors.) Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... webrtc/base/bufferqueue.cc:50: size_t next_packet_size = packet->size(); On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Consider using > > bytes = std::min(packet->size(), bytes) > > for legibility. (I realise this is outside of current CL.) Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... webrtc/base/bufferqueue.cc:93: void BufferQueue::PostEvent(Thread* t, int events) { On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Nit: t => thread No longer applies (removed). https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: // BufferEvents are used to asynchronously signal state transitionss. The flags On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Nit: "transitionss" is misspelled. No longer applies (removed). https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:39: enum { On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Nit: Define enum on one line for consistency. Please use "git cl format" to make > these sort of things consistent. No longer applies (removed). https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:45: // Creates a buffer queue queue with a given capacity, default buffer size On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Nit: "queue queue" Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:54: // that don't fit in the passed memory. On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Please add "Returns true unless no data could be returned." or something with a > similar meaning. Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:67: // re-entrancy. On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > I don't understand the 2nd sentence here. What does "unroll the stack" mean? By > "prevent re-entrancy", do you mean that this makes sure non-reentrant code is > not re-entered? (Which code?) By posting the event to a different (or the same) thread, it will be handled the next time this thread processes the events. This allows the method to return immediately and prevents the stack from getting too large (if the event handler calls another method which triggers an event, which calls another method, etc.). https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:73: // MessageHandler Interface On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Nit: The code style asks for a full stop after every comment. Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:82: mutable CriticalSection crit_; // object lock On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Consider using GUARDED_BY here. > See e.g. webrtc/call/rtc_event_log.cc for an example if its use. > > Nit: The comment "object lock" seems unnecessary. If you think it is, please > start with Capital letter and end with full stop. Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:157: virtual rtc::StreamResult WriteData(const void* data, size_t data_len, On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Please omit "virtual" for override methods (here and in several places below). Done. https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:228: LOG(LS_INFO) << "Closing outbound stream"; On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > Nit: Not a stream. :-) That was the comment of the original method, also it implements "StreamInterface", so I'd say it's a stream ;-) https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:239: static const int kBufferCapacity = 1; On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > I thought capacity >= size should hold for these buffers. What does it mean that > kBufferCapacity = 1 here? It's the size of the BufferQueue. If the caller tries to add more data, it will be "blocking" (i.e. returns "SR_BLOCK"). Once the caller read the data, the queue will become "writable" again and dispatches the corresponding event (see "OnEventOut"). For the tests it doesn't really matter what the size of the queue is. > Related to the buffer sizes is code in SSLStreamAdapterTestDTLS::ReadData and > ::WriteData. We should lift out their sizes, now oddly and inconsistently > hardwired to 1600 (allocation in WriteData), 1000 (actually hardwired but via > packet_size_), and 2000 (hardwired in ReadData). > > The data handled by BufferQueue is encrypted and slightly expanded wrt > non-encrypted data. The buffer sizes need to account for that. > > (I am aware that these hardwired sizes were there prior to your changes.) Agreed that the sizes should probably be adjusted. If so in a different CL.
On 2015/11/16 21:30:16, joachim wrote: > Ok, this got larger than I wanted it to be ;-) > > Anyways, BufferQueue now implements StreamInterface which made the test code a > lot cleaner (actually wanted to do that in a separate CL). > > Also made more changes based on feedback from torbjorng. > > Ptal > > https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h > File webrtc/base/bufferqueue.h (right): > > https://codereview.webrtc.org/1440193002/diff/1/webrtc/base/bufferqueue.h#new... > webrtc/base/bufferqueue.h:79: Thread* owner_; > On 2015/11/13 08:38:12, tommi (webrtc) wrote: > > if the value of this never changes, lets make it Thread* const owner_; > > What about calling it owning_thread_, main_thread_ or ctor_thread_ instead of > > owner_? > > Renamed, but can't change to "const" ("PostEvent" needs a non-const Thread). > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.cc > File webrtc/base/bufferqueue.cc (right): > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... > webrtc/base/bufferqueue.cc:21: : capacity_(capacity), > default_size_(default_size), owner_(owner) { > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Consider combining the two constructors, by using a default argument, i.e., > ..., > > Thread owner = Thread::Current(). > > > > (Default arguments are good for ctors.) > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... > webrtc/base/bufferqueue.cc:50: size_t next_packet_size = packet->size(); > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Consider using > > > > bytes = std::min(packet->size(), bytes) > > > > for legibility. (I realise this is outside of current CL.) > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.c... > webrtc/base/bufferqueue.cc:93: void BufferQueue::PostEvent(Thread* t, int > events) { > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Nit: t => thread > > No longer applies (removed). > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h > File webrtc/base/bufferqueue.h (right): > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:26: // BufferEvents are used to asynchronously signal > state transitionss. The flags > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Nit: "transitionss" is misspelled. > > No longer applies (removed). > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:39: enum { > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Nit: Define enum on one line for consistency. Please use "git cl format" to > make > > these sort of things consistent. > > No longer applies (removed). > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:45: // Creates a buffer queue queue with a given > capacity, default buffer size > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Nit: "queue queue" > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:54: // that don't fit in the passed memory. > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Please add "Returns true unless no data could be returned." or something with > a > > similar meaning. > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:67: // re-entrancy. > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > I don't understand the 2nd sentence here. What does "unroll the stack" mean? > By > > "prevent re-entrancy", do you mean that this makes sure non-reentrant code is > > not re-entered? (Which code?) > > By posting the event to a different (or the same) thread, it will be handled the > next time this thread processes the events. This allows the method to return > immediately and prevents the stack from getting too large (if the event handler > calls another method which triggers an event, which calls another method, etc.). > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:73: // MessageHandler Interface > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Nit: The code style asks for a full stop after every comment. > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/bufferqueue.h... > webrtc/base/bufferqueue.h:82: mutable CriticalSection crit_; // object lock > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Consider using GUARDED_BY here. > > See e.g. webrtc/call/rtc_event_log.cc for an example if its use. > > > > Nit: The comment "object lock" seems unnecessary. If you think it is, please > > start with Capital letter and end with full stop. > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... > File webrtc/base/sslstreamadapter_unittest.cc (right): > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... > webrtc/base/sslstreamadapter_unittest.cc:157: virtual rtc::StreamResult > WriteData(const void* data, size_t data_len, > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Please omit "virtual" for override methods (here and in several places below). > > Done. > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... > webrtc/base/sslstreamadapter_unittest.cc:228: LOG(LS_INFO) << "Closing outbound > stream"; > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > Nit: Not a stream. :-) > > That was the comment of the original method, also it implements > "StreamInterface", so I'd say it's a stream ;-) > > https://codereview.webrtc.org/1440193002/diff/20001/webrtc/base/sslstreamadap... > webrtc/base/sslstreamadapter_unittest.cc:239: static const int kBufferCapacity = > 1; > On 2015/11/16 14:05:03, torbjorng (webrtc) wrote: > > I thought capacity >= size should hold for these buffers. What does it mean > that > > kBufferCapacity = 1 here? > > It's the size of the BufferQueue. If the caller tries to add more data, it will > be "blocking" (i.e. returns "SR_BLOCK"). Once the caller read the data, the > queue will become "writable" again and dispatches the corresponding event (see > "OnEventOut"). > > For the tests it doesn't really matter what the size of the queue is. > > > Related to the buffer sizes is code in SSLStreamAdapterTestDTLS::ReadData and > > ::WriteData. We should lift out their sizes, now oddly and inconsistently > > hardwired to 1600 (allocation in WriteData), 1000 (actually hardwired but via > > packet_size_), and 2000 (hardwired in ReadData). > > > > The data handled by BufferQueue is encrypted and slightly expanded wrt > > non-encrypted data. The buffer sizes need to account for that. > > > > (I am aware that these hardwired sizes were there prior to your changes.) > > Agreed that the sizes should probably be adjusted. If so in a different CL. Ping? Would be nice to get the tests running again.
https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: class BufferQueue : public StreamInterface { I'm not sure about this change. It adds a lot of complexity to this class which is basically a utility class. Fwict, BufferQueue is only used by StreamInterfaceChannel which actually implements StreamInterface. (is that correct?) So, now StreamInterfaceChannel class is going to embed an object that implements the same interface effectively duplicating all the complexities that come with inheriting from the interface. StreamInterface is not a pure interface and actually includes a lot of default implementation (sigslot is hefty). StreamInterface in addition inherits from MessageHandler, which in turn pulls in a lot of message loop dependencies. All in all, that is too much in my opinion and I would much rather have these details moved closer to the code that's more dtls specific. Lastly, and this is a blocker for this CL as is, BufferQueue is a part of rtc_base_approved and that target cannot have these dependencies.
https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: class BufferQueue : public StreamInterface { On 2015/11/18 10:10:45, tommi (webrtc) wrote: > I'm not sure about this change. It adds a lot of complexity to this class which > is basically a utility class. > > Fwict, BufferQueue is only used by StreamInterfaceChannel which actually > implements StreamInterface. (is that correct?) Yes. With this change, StreamInterfaceChannel could now inherit from BufferQueue. > So, now StreamInterfaceChannel class is going to embed an object that implements > the same interface effectively duplicating all the complexities that come with > inheriting from the interface. StreamInterface is not a pure interface and > actually includes a lot of default implementation (sigslot is hefty). > StreamInterface in addition inherits from MessageHandler, which in turn pulls in > a lot of message loop dependencies. > > All in all, that is too much in my opinion and I would much rather have these > details moved closer to the code that's more dtls specific. > > Lastly, and this is a blocker for this CL as is, BufferQueue is a part of > rtc_base_approved and that target cannot have these dependencies. I see. What is the best way to implement event notifications on state changes then? From what I see, both sigslot.h and bind.h are not part of rtc_approved and can not be used, right? At least for fixing the tests, I need a way for users of BufferQueue to get notified about state changes ("now readable/writable").
https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: class BufferQueue : public StreamInterface { On 2015/11/18 10:33:58, joachim wrote: > On 2015/11/18 10:10:45, tommi (webrtc) wrote: > > I'm not sure about this change. It adds a lot of complexity to this class > which > > is basically a utility class. > > > > Fwict, BufferQueue is only used by StreamInterfaceChannel which actually > > implements StreamInterface. (is that correct?) > > Yes. With this change, StreamInterfaceChannel could now inherit from > BufferQueue. > > > So, now StreamInterfaceChannel class is going to embed an object that > implements > > the same interface effectively duplicating all the complexities that come with > > inheriting from the interface. StreamInterface is not a pure interface and > > actually includes a lot of default implementation (sigslot is hefty). > > StreamInterface in addition inherits from MessageHandler, which in turn pulls > in > > a lot of message loop dependencies. > > > > All in all, that is too much in my opinion and I would much rather have these > > details moved closer to the code that's more dtls specific. > > > > Lastly, and this is a blocker for this CL as is, BufferQueue is a part of > > rtc_base_approved and that target cannot have these dependencies. > > I see. What is the best way to implement event notifications on state changes > then? From what I see, both sigslot.h and bind.h are not part of rtc_approved > and can not be used, right? > > At least for fixing the tests, I need a way for users of BufferQueue to get > notified about state changes ("now readable/writable"). What about I add empty methods to the BufferQueue like this class BufferQueue { public: ... protected: virtual void NotifyReadable() {} virtual void NotifyWritable() {} }; Then override the class in the tests (and also inherit from StreamInterface there) and implement the methods to signal the events through StreamInterface. That way we don't introduce additional dependencies on BufferQueue but have the same functionality in the tests.
https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: class BufferQueue : public StreamInterface { On 2015/11/18 11:26:06, joachim wrote: > On 2015/11/18 10:33:58, joachim wrote: > > On 2015/11/18 10:10:45, tommi (webrtc) wrote: > > > I'm not sure about this change. It adds a lot of complexity to this class > > which > > > is basically a utility class. > > > > > > Fwict, BufferQueue is only used by StreamInterfaceChannel which actually > > > implements StreamInterface. (is that correct?) > > > > Yes. With this change, StreamInterfaceChannel could now inherit from > > BufferQueue. > > > > > So, now StreamInterfaceChannel class is going to embed an object that > > implements > > > the same interface effectively duplicating all the complexities that come > with > > > inheriting from the interface. StreamInterface is not a pure interface and > > > actually includes a lot of default implementation (sigslot is hefty). > > > StreamInterface in addition inherits from MessageHandler, which in turn > pulls > > in > > > a lot of message loop dependencies. > > > > > > All in all, that is too much in my opinion and I would much rather have > these > > > details moved closer to the code that's more dtls specific. > > > > > > Lastly, and this is a blocker for this CL as is, BufferQueue is a part of > > > rtc_base_approved and that target cannot have these dependencies. > > > > I see. What is the best way to implement event notifications on state changes > > then? From what I see, both sigslot.h and bind.h are not part of rtc_approved > > and can not be used, right? > > > > At least for fixing the tests, I need a way for users of BufferQueue to get > > notified about state changes ("now readable/writable"). > > What about I add empty methods to the BufferQueue like this > > class BufferQueue { > public: > ... > > protected: > virtual void NotifyReadable() {} > virtual void NotifyWritable() {} > }; > > Then override the class in the tests (and also inherit from StreamInterface > there) and implement the methods to signal the events through StreamInterface. > > That way we don't introduce additional dependencies on BufferQueue but have the > same functionality in the tests. If these methods are only needed for testing purposes, can we name them NotifyReadableForTest()? Alternatively, since the test can then presumably inject its own implementation of BufferQueue, the read/write methods could be made virtual, overridden in the test and the test implementation does the check to see if readable/writable changes. Having protected getters to read the state (e.g. get_queue()) might be necessary.
https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h File webrtc/base/bufferqueue.h (right): https://codereview.webrtc.org/1440193002/diff/60001/webrtc/base/bufferqueue.h... webrtc/base/bufferqueue.h:26: class BufferQueue : public StreamInterface { On 2015/11/18 12:20:04, tommi (webrtc) wrote: > On 2015/11/18 11:26:06, joachim wrote: > > On 2015/11/18 10:33:58, joachim wrote: > > > On 2015/11/18 10:10:45, tommi (webrtc) wrote: > > > > I'm not sure about this change. It adds a lot of complexity to this class > > > which > > > > is basically a utility class. > > > > > > > > Fwict, BufferQueue is only used by StreamInterfaceChannel which actually > > > > implements StreamInterface. (is that correct?) > > > > > > Yes. With this change, StreamInterfaceChannel could now inherit from > > > BufferQueue. > > > > > > > So, now StreamInterfaceChannel class is going to embed an object that > > > implements > > > > the same interface effectively duplicating all the complexities that come > > with > > > > inheriting from the interface. StreamInterface is not a pure interface > and > > > > actually includes a lot of default implementation (sigslot is hefty). > > > > StreamInterface in addition inherits from MessageHandler, which in turn > > pulls > > > in > > > > a lot of message loop dependencies. > > > > > > > > All in all, that is too much in my opinion and I would much rather have > > these > > > > details moved closer to the code that's more dtls specific. > > > > > > > > Lastly, and this is a blocker for this CL as is, BufferQueue is a part of > > > > rtc_base_approved and that target cannot have these dependencies. > > > > > > I see. What is the best way to implement event notifications on state > changes > > > then? From what I see, both sigslot.h and bind.h are not part of > rtc_approved > > > and can not be used, right? > > > > > > At least for fixing the tests, I need a way for users of BufferQueue to get > > > notified about state changes ("now readable/writable"). > > > > What about I add empty methods to the BufferQueue like this > > > > class BufferQueue { > > public: > > ... > > > > protected: > > virtual void NotifyReadable() {} > > virtual void NotifyWritable() {} > > }; > > > > Then override the class in the tests (and also inherit from StreamInterface > > there) and implement the methods to signal the events through StreamInterface. > > > > That way we don't introduce additional dependencies on BufferQueue but have > the > > same functionality in the tests. > > If these methods are only needed for testing purposes, can we name them > NotifyReadableForTest()? Done. > Alternatively, since the test can then presumably inject its own implementation > of BufferQueue, the read/write methods could be made virtual, overridden in the > test and the test implementation does the check to see if readable/writable > changes. Having protected getters to read the state (e.g. get_queue()) might be > necessary. The checks should be done inside the criticalsection of the queue to not lose events if different threads use the queue, so that would also need to get exposed to subclasses. I therefore implemented the protected methods for now. Please let me know what you think.
thanks. lgtm.
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440193002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440193002/80001
lgtm Two minor comments. Since this now is in the CQ, I can fix these in a separate CL. https://codereview.webrtc.org/1440193002/diff/80001/webrtc/base/sslstreamadap... File webrtc/base/sslstreamadapter_unittest.cc (right): https://codereview.webrtc.org/1440193002/diff/80001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:258: virtual void SetUp() override { Nit: Remove virtual (here and in 4 more places). https://codereview.webrtc.org/1440193002/diff/80001/webrtc/base/sslstreamadap... webrtc/base/sslstreamadapter_unittest.cc:268: if (!client_cert_pem_.empty() && !client_private_key_pem_.empty()) { Nit: With your slightly changed logic around PEM strings, perhaps it would be cleaner to pass NULL instead of empty strings for the corresponding params?
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 jbauch@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440193002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440193002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e488a0dbe4114ce51feeaf663ad4e2a6bd4b9a2b Cr-Commit-Position: refs/heads/master@{#10709}
Message was sent while issue was closed.
On 2015/11/19 13:18:16, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as > https://crrev.com/e488a0dbe4114ce51feeaf663ad4e2a6bd4b9a2b > Cr-Commit-Position: refs/heads/master@{#10709} fyi - the general rule is to wait for all reviewers to give approval.
Message was sent while issue was closed.
On 2015/11/19 13:46:14, tommi (webrtc) wrote: > On 2015/11/19 13:18:16, commit-bot: I haz the power wrote: > > Patchset 5 (id:??) landed as > > https://crrev.com/e488a0dbe4114ce51feeaf663ad4e2a6bd4b9a2b > > Cr-Commit-Position: refs/heads/master@{#10709} > > fyi - the general rule is to wait for all reviewers to give approval. Sorry, will wait for that in the future.
Message was sent while issue was closed.
On 2015/11/19 13:47:27, joachim wrote: > On 2015/11/19 13:46:14, tommi (webrtc) wrote: > > On 2015/11/19 13:18:16, commit-bot: I haz the power wrote: > > > Patchset 5 (id:??) landed as > > > https://crrev.com/e488a0dbe4114ce51feeaf663ad4e2a6bd4b9a2b > > > Cr-Commit-Position: refs/heads/master@{#10709} > > > > fyi - the general rule is to wait for all reviewers to give approval. > > Sorry, will wait for that in the future. no biggie - thanks for the fix :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
