Skip to content

Commit db304bf

Browse files
author
Peter Thorson
committed
Remove the use of cached read and write handlers in the asio transport. references zaphoyd#490 zaphoyd#525
There isn't a clean way to implement this performance optimization without adding global state/locking, which performs worse. This should fix
1 parent 9dc013a commit db304bf

File tree

2 files changed

+67
-104
lines changed

2 files changed

+67
-104
lines changed

‎changelog.md

+6
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,12 @@ HEAD
4646
Bastien Brunnenstein for reporting and a patch. #462
4747
- Bug: Fix an issue where TLS includes were broken for Asio Standalone builds.
4848
Thank you giachi and Bastien Brunnenstein for reporting. #491
49+
- Bug: Remove the use of cached read and write handlers in the Asio transport.
50+
This feature caused memory leaks when the io_service the connection was
51+
running on was abruptly stopped. There isn't a clean and safe way of using
52+
this optimization without global state and the associated locks. The locks
53+
perform worse. Thank you Xavier Gibert for reporting, test cases, and code.
54+
Fixes #490.
4955
- Compatibility: Fixes a number of build & config issues on Visual Studio 2015
5056
- Compatibility: Removes non-standards compliant masking behavior. #395, #469
5157
- Compatibility: Replace deprecated use of auto_ptr on systems where unique_ptr

‎websocketpp/transport/asio/connection.hpp

+61-104
Original file line numberDiff line numberDiff line change
@@ -415,12 +415,11 @@ class connection : public config::socket_type::socket_con_type {
415415
// TODO: pre-init timeout. Right now no implemented socket policies
416416
// actually have an asyncronous pre-init
417417

418-
m_init_handler = callback;
419-
420418
socket_con_type::pre_init(
421419
lib::bind(
422420
&type::handle_pre_init,
423421
get_shared(),
422+
callback,
424423
lib::placeholders::_1
425424
)
426425
);
@@ -465,26 +464,14 @@ class connection : public config::socket_type::socket_con_type {
465464
m_strand = lib::make_shared<lib::asio::io_service::strand>(
466465
lib::ref(*io_service));
467466
}
468-
m_async_read_handler = lib::bind(&type::handle_async_read,
469-
get_shared(), lib::placeholders::_1, lib::placeholders::_2);
470-
471-
m_async_write_handler = lib::bind(&type::handle_async_write,
472-
get_shared(), lib::placeholders::_1, lib::placeholders::_2);
473467

474468
lib::error_code ec = socket_con_type::init_asio(io_service, m_strand,
475469
m_is_server);
476470

477-
if (ec) {
478-
// reset the handlers to break the circular reference:
479-
// this->handler->this
480-
lib::clear_function(m_async_read_handler);
481-
lib::clear_function(m_async_write_handler);
482-
}
483-
484471
return ec;
485472
}
486473

487-
void handle_pre_init(lib::error_code const & ec) {
474+
void handle_pre_init(init_handler callback, lib::error_code const & ec) {
488475
if (m_alog.static_test(log::alevel::devel)) {
489476
m_alog.write(log::alevel::devel,"asio connection handle pre_init");
490477
}
@@ -494,19 +481,19 @@ class connection : public config::socket_type::socket_con_type {
494481
}
495482

496483
if (ec) {
497-
m_init_handler(ec);
484+
callback(ec);
498485
}
499486

500487
// If we have a proxy set issue a proxy connect, otherwise skip to
501488
// post_init
502489
if (!m_proxy.empty()) {
503-
proxy_write();
490+
proxy_write(callback);
504491
} else {
505-
post_init();
492+
post_init(callback);
506493
}
507494
}
508495

509-
void post_init() {
496+
void post_init(init_handler callback) {
510497
if (m_alog.static_test(log::alevel::devel)) {
511498
m_alog.write(log::alevel::devel,"asio connection post_init");
512499
}
@@ -520,7 +507,7 @@ class connection : public config::socket_type::socket_con_type {
520507
&type::handle_post_init_timeout,
521508
get_shared(),
522509
post_timer,
523-
m_init_handler,
510+
callback,
524511
lib::placeholders::_1
525512
)
526513
);
@@ -531,7 +518,7 @@ class connection : public config::socket_type::socket_con_type {
531518
&type::handle_post_init,
532519
get_shared(),
533520
post_timer,
534-
m_init_handler,
521+
callback,
535522
lib::placeholders::_1
536523
)
537524
);
@@ -607,15 +594,15 @@ class connection : public config::socket_type::socket_con_type {
607594
callback(ec);
608595
}
609596

610-
void proxy_write() {
597+
void proxy_write(init_handler callback) {
611598
if (m_alog.static_test(log::alevel::devel)) {
612599
m_alog.write(log::alevel::devel,"asio connection proxy_write");
613600
}
614601

615602
if (!m_proxy_data) {
616603
m_elog.write(log::elevel::library,
617604
"assertion failed: !m_proxy_data in asio::connection::proxy_write");
618-
m_init_handler(make_error_code(error::general));
605+
callback(make_error_code(error::general));
619606
return;
620607
}
621608

@@ -632,7 +619,7 @@ class connection : public config::socket_type::socket_con_type {
632619
lib::bind(
633620
&type::handle_proxy_timeout,
634621
get_shared(),
635-
m_init_handler,
622+
callback,
636623
lib::placeholders::_1
637624
)
638625
);
@@ -644,7 +631,7 @@ class connection : public config::socket_type::socket_con_type {
644631
m_bufs,
645632
m_strand->wrap(lib::bind(
646633
&type::handle_proxy_write, get_shared(),
647-
m_init_handler,
634+
callback,
648635
lib::placeholders::_1
649636
))
650637
);
@@ -654,7 +641,7 @@ class connection : public config::socket_type::socket_con_type {
654641
m_bufs,
655642
lib::bind(
656643
&type::handle_proxy_write, get_shared(),
657-
m_init_handler,
644+
callback,
658645
lib::placeholders::_1
659646
)
660647
);
@@ -825,15 +812,11 @@ class connection : public config::socket_type::socket_con_type {
825812
m_proxy_data.reset();
826813

827814
// Continue with post proxy initialization
828-
post_init();
815+
post_init(callback);
829816
}
830817
}
831818

832819
/// read at least num_bytes bytes into buf and then call handler.
833-
/**
834-
*
835-
*
836-
*/
837820
void async_read_at_least(size_t num_bytes, char *buf, size_t len,
838821
read_handler handler)
839822
{
@@ -843,13 +826,6 @@ class connection : public config::socket_type::socket_con_type {
843826
m_alog.write(log::alevel::devel,s.str());
844827
}
845828

846-
if (!m_async_read_handler) {
847-
m_alog.write(log::alevel::devel,
848-
"async_read_at_least called after async_shutdown");
849-
handler(make_error_code(transport::error::action_after_shutdown),0);
850-
return;
851-
}
852-
853829
// TODO: safety vs speed ?
854830
// maybe move into an if devel block
855831
/*if (num_bytes > len) {
@@ -860,22 +836,19 @@ class connection : public config::socket_type::socket_con_type {
860836
return;
861837
}*/
862838

863-
m_read_handler = handler;
864-
865-
if (!m_read_handler) {
866-
m_alog.write(log::alevel::devel,
867-
"asio con async_read_at_least called with bad handler");
868-
}
869-
870839
if (config::enable_multithreading) {
871840
lib::asio::async_read(
872841
socket_con_type::get_socket(),
873842
lib::asio::buffer(buf,len),
874843
lib::asio::transfer_at_least(num_bytes),
875-
m_strand->wrap(
876-
make_custom_alloc_handler(
877-
m_read_handler_allocator,
878-
m_async_read_handler))
844+
m_strand->wrap(make_custom_alloc_handler(
845+
m_read_handler_allocator,
846+
lib::bind(
847+
&type::handle_async_read, get_shared(),
848+
handler,
849+
lib::placeholders::_1, lib::placeholders::_2
850+
)
851+
))
879852
);
880853
} else {
881854
lib::asio::async_read(
@@ -884,14 +857,18 @@ class connection : public config::socket_type::socket_con_type {
884857
lib::asio::transfer_at_least(num_bytes),
885858
make_custom_alloc_handler(
886859
m_read_handler_allocator,
887-
m_async_read_handler
860+
lib::bind(
861+
&type::handle_async_read, get_shared(),
862+
handler,
863+
lib::placeholders::_1, lib::placeholders::_2
864+
)
888865
)
889866
);
890867
}
891868

892869
}
893870

894-
void handle_async_read(lib::asio::error_code const & ec,
871+
void handle_async_read(read_handler handler, lib::asio::error_code const & ec,
895872
size_t bytes_transferred)
896873
{
897874
m_alog.write(log::alevel::devel, "asio con handle_async_read");
@@ -915,10 +892,8 @@ class connection : public config::socket_type::socket_con_type {
915892
log_err(log::elevel::info,"asio async_read_at_least",ec);
916893
}
917894
}
918-
if (m_read_handler) {
919-
m_read_handler(tec,bytes_transferred);
920-
// TODO: why does this line break things?
921-
//m_read_handler = _WEBSOCKETPP_NULL_FUNCTION_;
895+
if (handler) {
896+
handler(tec,bytes_transferred);
922897
} else {
923898
// This can happen in cases where the connection is terminated while
924899
// the transport is waiting on a read.
@@ -927,70 +902,71 @@ class connection : public config::socket_type::socket_con_type {
927902
}
928903
}
929904

905+
/// Initiate a potentially asyncronous write of the given buffer
930906
void async_write(const char* buf, size_t len, write_handler handler) {
931-
if (!m_async_write_handler) {
932-
m_alog.write(log::alevel::devel,
933-
"async_write (single) called after async_shutdown");
934-
handler(make_error_code(transport::error::action_after_shutdown));
935-
return;
936-
}
937-
938907
m_bufs.push_back(lib::asio::buffer(buf,len));
939908

940-
m_write_handler = handler;
941-
942909
if (config::enable_multithreading) {
943910
lib::asio::async_write(
944911
socket_con_type::get_socket(),
945912
m_bufs,
946-
m_strand->wrap(
947-
make_custom_alloc_handler(
948-
m_write_handler_allocator,
949-
m_async_write_handler))
913+
m_strand->wrap(make_custom_alloc_handler(
914+
m_write_handler_allocator,
915+
lib::bind(
916+
&type::handle_async_write, get_shared(),
917+
handler,
918+
lib::placeholders::_1, lib::placeholders::_2
919+
)
920+
))
950921
);
951922
} else {
952923
lib::asio::async_write(
953924
socket_con_type::get_socket(),
954925
m_bufs,
955926
make_custom_alloc_handler(
956927
m_write_handler_allocator,
957-
m_async_write_handler
928+
lib::bind(
929+
&type::handle_async_write, get_shared(),
930+
handler,
931+
lib::placeholders::_1, lib::placeholders::_2
932+
)
958933
)
959934
);
960935
}
961936
}
962937

938+
/// Initiate a potentially asyncronous write of the given buffers
963939
void async_write(std::vector<buffer> const & bufs, write_handler handler) {
964-
if (!m_async_write_handler) {
965-
m_alog.write(log::alevel::devel,
966-
"async_write (vector) called after async_shutdown");
967-
handler(make_error_code(transport::error::action_after_shutdown));
968-
return;
969-
}
970940
std::vector<buffer>::const_iterator it;
971941

972942
for (it = bufs.begin(); it != bufs.end(); ++it) {
973943
m_bufs.push_back(lib::asio::buffer((*it).buf,(*it).len));
974944
}
975945

976-
m_write_handler = handler;
977-
978946
if (config::enable_multithreading) {
979947
lib::asio::async_write(
980948
socket_con_type::get_socket(),
981949
m_bufs,
982-
m_strand->wrap(
983-
make_custom_alloc_handler(
984-
m_write_handler_allocator,
985-
m_async_write_handler))
950+
m_strand->wrap(make_custom_alloc_handler(
951+
m_write_handler_allocator,
952+
lib::bind(
953+
&type::handle_async_write, get_shared(),
954+
handler,
955+
lib::placeholders::_1, lib::placeholders::_2
956+
)
957+
))
986958
);
987959
} else {
988960
lib::asio::async_write(
989961
socket_con_type::get_socket(),
990962
m_bufs,
991963
make_custom_alloc_handler(
992964
m_write_handler_allocator,
993-
m_async_write_handler
965+
lib::bind(
966+
&type::handle_async_write, get_shared(),
967+
handler,
968+
lib::placeholders::_1, lib::placeholders::_2
969+
)
994970
)
995971
);
996972
}
@@ -1001,17 +977,15 @@ class connection : public config::socket_type::socket_con_type {
1001977
* @param ec The status code
1002978
* @param bytes_transferred The number of bytes read
1003979
*/
1004-
void handle_async_write(lib::asio::error_code const & ec, size_t) {
980+
void handle_async_write(write_handler handler, lib::asio::error_code const & ec, size_t) {
1005981
m_bufs.clear();
1006982
lib::error_code tec;
1007983
if (ec) {
1008984
log_err(log::elevel::info,"asio async_write",ec);
1009985
tec = make_error_code(transport::error::pass_through);
1010986
}
1011-
if (m_write_handler) {
1012-
m_write_handler(tec);
1013-
// TODO: why does this line break things?
1014-
//m_write_handler = _WEBSOCKETPP_NULL_FUNCTION_;
987+
if (handler) {
988+
handler(tec);
1015989
} else {
1016990
// This can happen in cases where the connection is terminated while
1017991
// the transport is waiting on a read.
@@ -1064,16 +1038,6 @@ class connection : public config::socket_type::socket_con_type {
10641038
m_alog.write(log::alevel::devel,"asio connection async_shutdown");
10651039
}
10661040

1067-
// Reset cached handlers now that we won't be reading or writing anymore
1068-
// These cached handlers store shared pointers to this connection and
1069-
// will leak the connection if not destroyed.
1070-
lib::clear_function(m_async_read_handler);
1071-
lib::clear_function(m_async_write_handler);
1072-
lib::clear_function(m_init_handler);
1073-
1074-
lib::clear_function(m_read_handler);
1075-
lib::clear_function(m_write_handler);
1076-
10771041
timer_ptr shutdown_timer;
10781042
shutdown_timer = set_timer(
10791043
config::timeout_socket_shutdown,
@@ -1230,13 +1194,6 @@ class connection : public config::socket_type::socket_con_type {
12301194

12311195
handler_allocator m_read_handler_allocator;
12321196
handler_allocator m_write_handler_allocator;
1233-
1234-
read_handler m_read_handler;
1235-
write_handler m_write_handler;
1236-
init_handler m_init_handler;
1237-
1238-
async_read_handler m_async_read_handler;
1239-
async_write_handler m_async_write_handler;
12401197
};
12411198

12421199

0 commit comments

Comments
 (0)