Skip to content

Fixed some memory leaks with http/3#9336

Merged
bryancall merged 1 commit intoapache:10-Devfrom
bryancall:h3_memory_leaks1
Feb 1, 2023
Merged

Fixed some memory leaks with http/3#9336
bryancall merged 1 commit intoapache:10-Devfrom
bryancall:h3_memory_leaks1

Conversation

@bryancall
Copy link
Copy Markdown
Contributor

@bryancall bryancall commented Jan 27, 2023 •
edited
Loading

These numbers are with using freelists:

Before:
SUMMARY: AddressSanitizer: 9997094750 byte(s) leaked in 4587157 allocation(s).
After:
SUMMARY: AddressSanitizer: 137185838 byte(s) leaked in 77301 allocation(s).

@bryancall bryancall added this to the 10.0.0 milestone Jan 27, 2023
@bryancall bryancall requested a review from maskit January 27, 2023 19:29
@bryancall bryancall self-assigned this Jan 27, 2023
@bryancall
Copy link
Copy Markdown
Contributor Author

bryancall commented Jan 27, 2023

For reference this is where HQSession::remove_transaction is called:

Thread 12 "[ET_NET 9]" hit Breakpoint 1, Queue<HQTransaction, Continuation::Link_link>::remove (this=<optimized out>, e=<optimized out>) at /home/bcall/dev/apache/trafficserver/build-Linux_clang_h3_asan/../include/tscore/List.h:592
592       if (tail == e)
(gdb) bt
#0  Queue<HQTransaction, Continuation::Link_link>::remove (this=<optimized out>, e=<optimized out>)
    at /home/bcall/dev/apache/trafficserver/build-Linux_clang_h3_asan/../include/tscore/List.h:592
#1  HQSession::remove_transaction (this=0x620000010080, trans=0x626000012100) at ../../../proxy/http3/Http3Session.cc:57
#2  0x0000000000f59ebc in Http3App::_handle_bidi_stream_on_write_complete (this=<optimized out>, event=<optimized out>, vio=0x612000073170)
    at ../../../proxy/http3/Http3App.cc:354
#3  0x0000000000f57acd in Http3App::main_event_handler (this=0x62600000f100, event=103, data=<optimized out>) at ../../../proxy/http3/Http3App.cc:148
#4  0x0000000000fa9f5f in Continuation::handleEvent (this=<optimized out>, event=103, data=0x62c000004b40) at ../../../iocore/eventsystem/I_Continuation.h:227
#5  EThread::process_event (this=<optimized out>, e=0x62c000004b40, calling_code=<optimized out>) at ../../../iocore/eventsystem/UnixEThread.cc:152
#6  0x0000000000faae90 in EThread::process_queue (this=this@entry=0x7ffff0ad1800, NegativeQueue=NegativeQueue@entry=0x7fffe79400a0,
    ev_count=ev_count@entry=0x7fffe79400d0, nq_count=nq_count@entry=0x7fffe79400c0) at ../../../iocore/eventsystem/UnixEThread.cc:187
#7  0x0000000000fac572 in EThread::execute_regular (this=0x7ffff0ad1800) at ../../../iocore/eventsystem/UnixEThread.cc:265
#8  0x0000000000fadbcc in EThread::execute (this=0x7ffff0ad1800) at ../../../iocore/eventsystem/UnixEThread.cc:337
#9  0x0000000000fa78bc in spawn_thread_internal (a=0x606000040940) at ../../../iocore/eventsystem/Thread.cc:79
#10 0x00007ffff6cae12d in start_thread (arg=<optimized out>) at pthread_create.c:442
#11 0x00007ffff6d2fbc0 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

And Http3HeaderFramer::~Http3HeaderFramer:

Thread 12 "[ET_NET 9]" hit Breakpoint 2, Http3HeaderFramer::~Http3HeaderFramer (this=0x611000070fc0) at ../../../proxy/http3/Http3HeaderFramer.cc:40
40      {
(gdb) bt
#0  Http3HeaderFramer::~Http3HeaderFramer (this=0x611000070fc0) at ../../../proxy/http3/Http3HeaderFramer.cc:40
#1  0x0000000000f6329a in Http3Transaction::~Http3Transaction (this=0x626000012100) at ../../../proxy/http3/Http3Transaction.cc:324
#2  0x0000000000f635de in Http3Transaction::~Http3Transaction (this=0x611000070fc0) at ../../../proxy/http3/Http3Transaction.cc:323
#3  0x0000000000f59ebc in Http3App::_handle_bidi_stream_on_write_complete (this=<optimized out>, event=<optimized out>, vio=0x612000072870)
    at ../../../proxy/http3/Http3App.cc:354
#4  0x0000000000f57acd in Http3App::main_event_handler (this=0x62600000f100, event=103, data=<optimized out>) at ../../../proxy/http3/Http3App.cc:148
#5  0x0000000000fa9f5f in Continuation::handleEvent (this=<optimized out>, event=103, data=0x62c0000048a0) at ../../../iocore/eventsystem/I_Continuation.h:227
#6  EThread::process_event (this=<optimized out>, e=0x62c0000048a0, calling_code=<optimized out>) at ../../../iocore/eventsystem/UnixEThread.cc:152
#7  0x0000000000faae90 in EThread::process_queue (this=this@entry=0x7ffff0ad1800, NegativeQueue=NegativeQueue@entry=0x7fffe779c0a0,
    ev_count=ev_count@entry=0x7fffe779c0d0, nq_count=nq_count@entry=0x7fffe779c0c0) at ../../../iocore/eventsystem/UnixEThread.cc:187
#8  0x0000000000fac572 in EThread::execute_regular (this=0x7ffff0ad1800) at ../../../iocore/eventsystem/UnixEThread.cc:265
#9  0x0000000000fadbcc in EThread::execute (this=0x7ffff0ad1800) at ../../../iocore/eventsystem/UnixEThread.cc:337
#10 0x0000000000fa78bc in spawn_thread_internal (a=0x606000040940) at ../../../iocore/eventsystem/Thread.cc:79
#11 0x00007ffff6cae12d in start_thread (arg=<optimized out>) at pthread_create.c:442
#12 0x00007ffff6d2fbc0 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

maskit
maskit reviewed Jan 31, 2023
View reviewed changes
Comment thread proxy/http3/Http3Session.cc
HQSession::remove_transaction(HQTransaction *trans)
{
this->_transaction_list.remove(trans);
delete trans;
Copy link
Copy Markdown
Member

@maskit maskit Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I want to delete transactions on the caller side, but I'll propose the change later.

Copy link
Copy Markdown
Member

@maskit maskit Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but I wonder why it leaks. Because all transactions in _transaction_list are deleted in the destructor of HQSession. See #9346

Copy link
Copy Markdown
Contributor Author

@bryancall bryancall Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If HQSession::remove_transaction() is called it will remove the transaction from the _transaction_list and it will never be deleted.

Copy link
Copy Markdown
Member

@maskit maskit Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right. Go ahead and merge this.

@bryancall bryancall mentioned this pull request Jan 31, 2023
Merged
@bryancall bryancall added the Leak label Jan 31, 2023
@bryancall bryancall merged commit a776a9f into apache:10-Dev Feb 1, 2023
bryancall added a commit to bryancall/trafficserver that referenced this pull request Feb 3, 2023
@bryancall
Fixed some memory leaks with http/3 (apache#9336)
682c7ba 
(cherry picked from commit a776a9f)
@bryancall bryancall modified the milestones: 10.0.0, Sometime Feb 3, 2023
bryancall added a commit that referenced this pull request Feb 3, 2023
@bryancall
Fixed some memory leaks with http/3 (#9336) (#9375)
271038d 
(cherry picked from commit a776a9f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maskit maskit maskit approved these changes

Assignees

@bryancall bryancall

Labels

HTTP/3 Leak

Projects

None yet

Milestone

Sometime

Development

Successfully merging this pull request may close these issues.

2 participants

@bryancall @maskit