From e0bc4bcf032e273da0614ca1a625bf5b211c1437 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 17 Jun 2021 08:26:46 -0700 Subject: [PATCH 1/2] Fixed memory leak in the QUIC stream manager. Removed the proxy allocator for the streams. They can be added back in if there is a performance issue. --- iocore/eventsystem/I_Thread.h | 3 --- iocore/net/quic/QUICStreamFactory.cc | 36 ++++------------------------ iocore/net/quic/QUICStreamManager.h | 6 +++++ 3 files changed, 10 insertions(+), 35 deletions(-) diff --git a/iocore/eventsystem/I_Thread.h b/iocore/eventsystem/I_Thread.h index f2f1b1c2ccd..7ec7c3ebc01 100644 --- a/iocore/eventsystem/I_Thread.h +++ b/iocore/eventsystem/I_Thread.h @@ -123,9 +123,6 @@ class Thread ProxyAllocator http2ClientSessionAllocator; ProxyAllocator http2StreamAllocator; ProxyAllocator quicClientSessionAllocator; - ProxyAllocator quicBidiStreamAllocator; - ProxyAllocator quicSendStreamAllocator; - ProxyAllocator quicReceiveStreamAllocator; ProxyAllocator httpServerSessionAllocator; ProxyAllocator hdrHeapAllocator; ProxyAllocator strHeapAllocator; diff --git a/iocore/net/quic/QUICStreamFactory.cc b/iocore/net/quic/QUICStreamFactory.cc index 9762895eb78..548be1050fa 100644 --- a/iocore/net/quic/QUICStreamFactory.cc +++ b/iocore/net/quic/QUICStreamFactory.cc @@ -26,30 +26,20 @@ #include "QUICUnidirectionalStream.h" #include "QUICStreamFactory.h" -ClassAllocator quicBidiStreamAllocator("quicBidiStreamAllocator"); -ClassAllocator quicSendStreamAllocator("quicSendStreamAllocator"); -ClassAllocator quicReceiveStreamAllocator("quicReceiveStreamAllocator"); - QUICStreamVConnection * QUICStreamFactory::create(QUICStreamId sid, uint64_t local_max_stream_data, uint64_t remote_max_stream_data) { QUICStreamVConnection *stream = nullptr; switch (QUICTypeUtil::detect_stream_direction(sid, this->_info->direction())) { case QUICStreamDirection::BIDIRECTIONAL: - // TODO Free the stream somewhere - stream = THREAD_ALLOC(quicBidiStreamAllocator, this_ethread()); - new (stream) QUICBidirectionalStream(this->_rtt_provider, this->_info, sid, local_max_stream_data, remote_max_stream_data); + stream = new QUICBidirectionalStream(this->_rtt_provider, this->_info, sid, local_max_stream_data, remote_max_stream_data); break; case QUICStreamDirection::SEND: - // TODO Free the stream somewhere - stream = THREAD_ALLOC(quicSendStreamAllocator, this_ethread()); - new (stream) QUICSendStream(this->_info, sid, remote_max_stream_data); + stream = new QUICSendStream(this->_info, sid, remote_max_stream_data); break; case QUICStreamDirection::RECEIVE: // server side - // TODO Free the stream somewhere - stream = THREAD_ALLOC(quicReceiveStreamAllocator, this_ethread()); - new (stream) QUICReceiveStream(this->_rtt_provider, this->_info, sid, local_max_stream_data); + stream = new QUICReceiveStream(this->_rtt_provider, this->_info, sid, local_max_stream_data); break; default: ink_assert(false); @@ -62,23 +52,5 @@ QUICStreamFactory::create(QUICStreamId sid, uint64_t local_max_stream_data, uint void QUICStreamFactory::delete_stream(QUICStreamVConnection *stream) { - if (!stream) { - return; - } - - stream->~QUICStreamVConnection(); - switch (stream->direction()) { - case QUICStreamDirection::BIDIRECTIONAL: - THREAD_FREE(static_cast(stream), quicBidiStreamAllocator, this_thread()); - break; - case QUICStreamDirection::SEND: - THREAD_FREE(static_cast(stream), quicSendStreamAllocator, this_thread()); - break; - case QUICStreamDirection::RECEIVE: - THREAD_FREE(static_cast(stream), quicReceiveStreamAllocator, this_thread()); - break; - default: - ink_assert(false); - break; - } + delete stream; } diff --git a/iocore/net/quic/QUICStreamManager.h b/iocore/net/quic/QUICStreamManager.h index 05da5fc93ab..6248a4bcc4a 100644 --- a/iocore/net/quic/QUICStreamManager.h +++ b/iocore/net/quic/QUICStreamManager.h @@ -40,6 +40,12 @@ class QUICStreamManager : public QUICFrameHandler, public QUICFrameGenerator { public: QUICStreamManager(QUICContext *context, QUICApplicationMap *app_map); + ~QUICStreamManager() + { + for (auto stream = stream_list.pop(); stream != nullptr; stream = stream_list.pop()) { + _stream_factory.delete_stream(stream); + } + } void init_flow_control_params(const std::shared_ptr &local_tp, const std::shared_ptr &remote_tp); From 0357aa8a579e434a307191cb7ebae4caf71496bf Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Thu, 17 Jun 2021 14:02:53 -0700 Subject: [PATCH 2/2] moved the destructor into the .cc file --- iocore/net/quic/QUICStreamManager.cc | 7 +++++++ iocore/net/quic/QUICStreamManager.h | 7 +------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/iocore/net/quic/QUICStreamManager.cc b/iocore/net/quic/QUICStreamManager.cc index da66d740822..54a6d20958b 100644 --- a/iocore/net/quic/QUICStreamManager.cc +++ b/iocore/net/quic/QUICStreamManager.cc @@ -41,6 +41,13 @@ QUICStreamManager::QUICStreamManager(QUICContext *context, QUICApplicationMap *a } } +QUICStreamManager::~QUICStreamManager() +{ + for (auto stream = stream_list.pop(); stream != nullptr; stream = stream_list.pop()) { + _stream_factory.delete_stream(stream); + } +} + std::vector QUICStreamManager::interests() { diff --git a/iocore/net/quic/QUICStreamManager.h b/iocore/net/quic/QUICStreamManager.h index 6248a4bcc4a..f158035a8aa 100644 --- a/iocore/net/quic/QUICStreamManager.h +++ b/iocore/net/quic/QUICStreamManager.h @@ -40,12 +40,7 @@ class QUICStreamManager : public QUICFrameHandler, public QUICFrameGenerator { public: QUICStreamManager(QUICContext *context, QUICApplicationMap *app_map); - ~QUICStreamManager() - { - for (auto stream = stream_list.pop(); stream != nullptr; stream = stream_list.pop()) { - _stream_factory.delete_stream(stream); - } - } + ~QUICStreamManager(); void init_flow_control_params(const std::shared_ptr &local_tp, const std::shared_ptr &remote_tp);