Fix DTLS packet boundary handling in SSLStreamAdapterTests.

webrtc/base/bufferqueue.h

 /*
  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_BASE_BUFFERQUEUE_H_
 #define WEBRTC_BASE_BUFFERQUEUE_H_
 
 #include <deque>
 #include <vector>
 
 #include "webrtc/base/buffer.h"
 #include "webrtc/base/criticalsection.h"
+#include "webrtc/base/messagehandler.h"
+#include "webrtc/base/messagequeue.h"
+#include "webrtc/base/stream.h"
+#include "webrtc/base/thread.h"
 
 namespace rtc {
 
-class BufferQueue {
+class BufferQueue : public StreamInterface {

[tommi, 2015/11/18 10:10:45]

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.

[joachim, 2015/11/18 10:33:58]

Yes. With this change, StreamInterfaceChannel could now inherit from
BufferQueue.
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").

[joachim, 2015/11/18 11:26:06]

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.

[tommi, 2015/11/18 12:20:04]

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.

[joachim, 2015/11/18 13:57:00]

Done.
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.

  public:
-  // Creates a buffer queue queue with a given capacity and default buffer size.
-  BufferQueue(size_t capacity, size_t default_size);
+  // Creates a buffer queue with a given capacity, default buffer size
+  // and owner thread.
+  BufferQueue(size_t capacity, size_t default_size,
+              Thread* owner = Thread::Current());
   ~BufferQueue();
 
   // Return number of queued buffers.
   size_t size() const;
 
   // ReadFront will only read one buffer at a time and will truncate buffers
   // that don't fit in the passed memory.
+  // Returns true unless no data could be returned.
   bool ReadFront(void* data, size_t bytes, size_t* bytes_read);
 
   // WriteBack always writes either the complete memory or nothing.
+  // Returns true unless no data could be written.
   bool WriteBack(const void* data, size_t bytes, size_t* bytes_written);
 
+  // Implementation of abstract StreamInterface methods.
+
+  // A buffer queue is always "open".
+  StreamState GetState() const override { return SS_OPEN; }
+
+  // Reading a buffer queue will either succeed or block.
+  StreamResult Read(void* buffer, size_t buffer_len,
+                    size_t* read, int* error) override {
+    if (!ReadFront(buffer, buffer_len, read)) {
+      return SR_BLOCK;
+    }
+    return SR_SUCCESS;
+  }
+
+  // Writing to a buffer queue will either succeed or block.
+  StreamResult Write(const void* data, size_t data_len,
+                     size_t* written, int* error) override {
+    if (!WriteBack(data, data_len, written)) {
+      return SR_BLOCK;
+    }
+    return SR_SUCCESS;
+  }
+
+  // A buffer queue can not be closed.
+  void Close() override {}
+
  private:
   size_t capacity_;
   size_t default_size_;
-  std::deque<Buffer*> queue_;
-  std::vector<Buffer*> free_list_;
-  mutable CriticalSection crit_;  // object lock
+  Thread* owner_thread_;
+  mutable CriticalSection crit_;
+  std::deque<Buffer*> queue_ GUARDED_BY(crit_);
+  std::vector<Buffer*> free_list_ GUARDED_BY(crit_);
 
   RTC_DISALLOW_COPY_AND_ASSIGN(BufferQueue);
 };
 
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_BUFFERQUEUE_H_