PS-5840 - Memory leak in after 'main.threadpool_debug' - in 'Thread_p…

[altmannmarcelo]

…ool_connection_handler::add_connection()'

Problem
If threadpool failed to allocate a connection, vio component allocated
as part of channel_info->create_thd() won't be freed.

Solution
call vio_delete on allocated vio to free up memory.
vio_delete will also close the channel, which particularly in this case
will be handled by send_error_and_close_channel, thus we set
vio->inactive= TRUE in order to avoid it been closed at vio_delete.

[altmannmarcelo]

https://ps57.cd.percona.com/job/percona-server-5.7-param/176/

[altmannmarcelo]

test after rebranch to latest GCA https://ps57.cd.percona.com/job/percona-server-5.7-pipeline/8465/

[percona-ysorokin]

@altmannmarcelo From what I see from other parts of the code, for instance here (

percona-server/plugin/innodb_memcached/innodb_memcache/src/handler_api.cc

Line 380 in c08e5cd

thd->get_protocol_classic()->end_net();

)
In patterns like this, we usually call thd->release_resources(); instead.
If this does not work, memory from vio object should not be deleted directly by my_free(). Use vio_delete() instead

[altmannmarcelo]

@percona-ysorokin I've re analyzed the issue based on your comments. I believe I got it right this time:

If threadpool failed to allocate a connection, vio component allocated
as part of channel_info->create_thd() won't be freed.
There were two issues in the current implementation:

• calling thd->get_protocol_classic()->end_net() alone. This should be
  called after vio_delete for proper cleanup, otherwise, it will only
  assign NULL to vio. However, for add_connection() we use
  send_error_and_close_channel, which will also attempt to close
  the socket that was already closed by vio_delete causing a crash.
• We are calling the destructor of THD. As part of the destructor,
  release_resources will be called, which in case still have
  a valid vio, will try to close the socket, leading to a crash when
  send_error_and_close_channel is called.

Solution
set vio->inactive= TRUE in order to avoid it been closed at vio_delete.
vio_delete will be called as part of THD destructor, freeing
the allocated resources. send_error_and_close_channel will report the
error to client and close the socket.

https://ps57.cd.percona.com/job/percona-server-5.7-pipeline/9073/

==============================================================================

TEST                                      RESULT   TIME (ms) or COMMENT
--------------------------------------------------------------------------

worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 13000..13009
[ 50%] main.threadpool_debug                    [ pass ]      6
[100%] unit_tests                               [ pass ]       
[150%] sanitize_report                          [ pass ]       
--------------------------------------------------------------------------

[percona-ysorokin]

LGTM