[DNM] Server-side support for UNIX-domain stream sockets #7

ronen-fr · 2019-08-08T13:40:55Z

Adding server-side support for UNIX-domain stream sockets (used by the admin console).
Implementation note:
A third 'transport' option was added to the existing TCP & SCTP.

Note: no support for abstract-namespace sockets. As A.N. sockets save us the need to handle the clean-up of the created filesystem objects, I plan to add required support in a future pull request.

(new version will be pushed shortly)

tchaikov · 2019-08-08T16:42:50Z

include/seastar/net/socket_defs.hh

@@ -77,7 +86,8 @@ std::ostream& operator<<(std::ostream&, const socket_address&);

 enum class transport {
    TCP = IPPROTO_TCP,
-    SCTP = IPPROTO_SCTP
+    SCTP = IPPROTO_SCTP,
+    UNIX 


i think it would be better to add a DEFAULT = 0, as AF_UNIX only support 0 as its protocol. see http://man7.org/linux/man-pages/man7/unix.7.html

Fixed to
enum class transport {
UNIX = 0,
TCP = IPPROTO_TCP,
SCTP = IPPROTO_SCTP
};
Note that this relies on IPPROTO_TCP to be <> 0 (which it is, but only by chance).
Indeed existing code uses the enum value for the actual protocol in the 'socket()' call, which I can't say I like.

please note DEFAULT is a protocol. while UNIX is not.

tchaikov · 2019-08-08T16:43:34Z

src/core/reactor.cc

-    }
-    if (_reuseport)
-        fd.setsockopt(SOL_SOCKET, SO_REUSEPORT, 1);
+    if (opts.proto == transport::UNIX) {


Suggested change

if (opts.proto == transport::UNIX) {

if (opts.proto.sa_family == AF_UNIX) {

how about checking sa.as_posix_sockaddr.sa_family ?

Existing seastar code usually checks the transport. And the code looks better (to my taste...)

again, see #7 (comment)

tchaikov · 2019-08-08T16:48:57Z

include/seastar/core/posix.hh

@@ -248,6 +248,10 @@ public:
        auto r = ::bind(_fd, &sa, sl);
        throw_system_error_on(r == -1, "bind");
    }
+    void unixdomain_bind(sockaddr_un sa) {


no need to add a dedicated unixdomain_bind() .

fixed. Note that I'll have to revisit that when adding support for abstract-namespace ud sockets.

tchaikov · 2019-08-08T16:54:29Z

src/net/posix-stack.cc

@@ -367,16 +405,22 @@ posix_data_sink_impl::close() {

 server_socket
 posix_network_stack::listen(socket_address sa, listen_options opt) {
-    if (opt.proto == transport::TCP) {
+    switch (opt.proto) {


i'd suggest check address family first then check the protocol. like

if (sa.sa_familty == AF_INET || sa.sa_familty == AF_INET6) { if (opt.proto == transport::TCP) { // ... } } else if (sa.sa_familty == AF_UNIX) { // ... }

Seastar code seems to prefer checking the transport, and I'd rather not deviate from that.
Also - code is clearer when using the 'switch'

@ronen-fr see #7 (comment) . the reason why i suggest use family is that local/unix socket is not a protocol. ideally, it should not be listed along with TCP or SCP. yeah, we could use switch for checking sa_family if it's better.

tchaikov · 2019-08-08T17:00:17Z

include/seastar/net/socket_defs.hh

@@ -53,13 +58,17 @@ public:
    socket_address(ipv4_addr);
    socket_address(const ipv6_addr&);
    socket_address(const net::inet_address&, uint16_t p = 0);
+    socket_address(const std::filesystem::path);


it's nice to support std::filesystem::path. but seastar supports C++14. so might want to use seastar::compat::filesystem::path ? or how about just using std::sstring ? as we are not using any specific feature of std::filesystem::path, right?

Indeed I am not using any feature. Will remove the fs::path.
Note that scylladb::seastar does use or <experimental/filesystem>

that's why i suggest use seastar::compat::filesystem::path as an option.

tchaikov · 2019-08-08T17:01:53Z

include/seastar/net/socket_defs.hh

@@ -123,6 +133,14 @@ struct ipv6_addr {
    }
 };

+struct ud_addr {
+    std::filesystem::path sfile_;


i'd suggest s/sfile_/path/. and just use std::string.

tchaikov · 2019-08-08T17:02:43Z

include/seastar/net/socket_defs.hh

+    std::filesystem::path sfile_;
+
+    explicit ud_addr(const std::string&);
+    explicit ud_addr(std::string_view fn) { sfile_ = fn; }


this requires C++17. use seastar::compat::string_view instead. or just drop this construct to be consistent with its cousins.

tchaikov · 2019-08-08T17:05:20Z

include/seastar/net/socket_defs.hh

+struct ud_addr {
+    std::filesystem::path sfile_;
+
+    explicit ud_addr(const std::string&);


the implementation of this constructor is not defined.

tchaikov · 2019-08-08T17:06:52Z

include/seastar/net/socket_defs.hh

+
+    explicit ud_addr(const std::string&);
+    explicit ud_addr(std::string_view fn) { sfile_ = fn; }
+    explicit ud_addr(const std::filesystem::path& fn) { sfile_ = fn; }


please use member initializer list to avoid creating an instance and then throw it away when assigning to it.

fixed - but note that I checked the resulting assembly both ways - and the optimizer produces the same code.

thanks for checking the assembly. but i'd rely on the behavior defined by standard as long as it's defined.

tchaikov · 2019-08-08T17:07:33Z

include/seastar/net/socket_defs.hh

@@ -123,6 +133,14 @@ struct ipv6_addr {
    }
 };

+struct ud_addr {


s/ud_addr/unix_domain_addr/

IMHO, a type as a part of interface deserves a better name.

This reverts commit 33406cf. It introduces memory leaks: Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x7fb773b389d7 in operator new(unsigned long) (/lib64/libasan.so.5+0x10f9d7) ceph#1 0x108f0d4 in seastar::reactor::poller::~poller() ../src/core/reactor.cc:2879 ceph#2 0x11c1e59 in std::experimental::fundamentals_v1::_Optional_base<seastar::reactor::poller, true>::~_Optional_base() /usr/include/c++/9/experimental/optional:288 ceph#3 0x118f2d7 in std::experimental::fundamentals_v1::optional<seastar::reactor::poller>::~optional() /usr/include/c++/9/experimental/optional:491 ceph#4 0x108c5a5 in seastar::reactor::run() ../src/core/reactor.cc:2587 ceph#5 0xf1a822 in seastar::app_template::run_deprecated(int, char**, std::function<void ()>&&) ../src/core/app-template.cc:199 ceph#6 0xf1885d in seastar::app_template::run(int, char**, std::function<seastar::future<int> ()>&&) ../src/core/app-template.cc:115 ceph#7 0xeb2735 in operator() ../src/testing/test_runner.cc:72 ceph#8 0xebb342 in _M_invoke /usr/include/c++/9/bits/std_function.h:300 ceph#9 0xf3d8b0 in std::function<void ()>::operator()() const /usr/include/c++/9/bits/std_function.h:690 ceph#10 0x1034c72 in seastar::posix_thread::start_routine(void*) ../src/core/posix.cc:52 ceph#11 0x7fb7738804e1 in start_thread /usr/src/debug/glibc-2.30-13-g919af705ee/nptl/pthread_create.c:479 Reported-by: Rafael Avila de Espindola <espindola@scylladb.com>

tchaikov · 2020-01-23T03:43:37Z

closing as it's already upstreamed.

…o_with Fixes failures in debug mode: ``` $ build/debug/tests/unit/closeable_test -l all -t deferred_close_test WARNING: debug mode. Not for benchmarking or production random-seed=3064133628 Running 1 test case... Entering test module "../../tests/unit/closeable_test.cc" ../../tests/unit/closeable_test.cc(0): Entering test case "deferred_close_test" ../../src/testing/seastar_test.cc(43): info: check true has passed ==9449==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! terminate called after throwing an instance of 'seastar::broken_promise' what(): broken promise ==9449==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7fbf1f49f000; bottom 0x7fbf40971000; size: 0xffffffffdeb2e000 (-558702592) False positive error reports may follow For details see google/sanitizers#189 ================================================================= ==9449==AddressSanitizer CHECK failed: ../../../../libsanitizer/asan/asan_thread.cpp:356 "((ptr[0] == kCurrentStackFrameMagic)) != (0)" (0x0, 0x0) #0 0x7fbf45f39d0b (/lib64/libasan.so.6+0xb3d0b) #1 0x7fbf45f57d4e (/lib64/libasan.so.6+0xd1d4e) #2 0x7fbf45f3e724 (/lib64/libasan.so.6+0xb8724) #3 0x7fbf45eb3e5b (/lib64/libasan.so.6+0x2de5b) #4 0x7fbf45eb51e8 (/lib64/libasan.so.6+0x2f1e8) #5 0x7fbf45eb7694 (/lib64/libasan.so.6+0x31694) #6 0x7fbf45f39398 (/lib64/libasan.so.6+0xb3398) #7 0x7fbf45f3a00b in __asan_report_load8 (/lib64/libasan.so.6+0xb400b) #8 0xfe6d52 in bool __gnu_cxx::operator!=<dl_phdr_info*, std::vector<dl_phdr_info, std::allocator<dl_phdr_info> > >(__gnu_cxx::__normal_iterator<dl_phdr_info*, std::vector<dl_phdr_info, std::allocator<dl_phdr_info> > > const&, __gnu_cxx::__normal_iterator<dl_phdr_info*, std::vector<dl_phdr_info, std::allocator<dl_phdr_info> > > const&) /usr/include/c++/10/bits/stl_iterator.h:1116 #9 0xfe615c in dl_iterate_phdr ../../src/core/exception_hacks.cc:121 #10 0x7fbf44bd1810 in _Unwind_Find_FDE (/lib64/libgcc_s.so.1+0x13810) #11 0x7fbf44bcd897 (/lib64/libgcc_s.so.1+0xf897) #12 0x7fbf44bcea5f (/lib64/libgcc_s.so.1+0x10a5f) #13 0x7fbf44bcefd8 in _Unwind_RaiseException (/lib64/libgcc_s.so.1+0x10fd8) #14 0xfe6281 in _Unwind_RaiseException ../../src/core/exception_hacks.cc:148 scylladb#15 0x7fbf457364bb in __cxa_throw (/lib64/libstdc++.so.6+0xaa4bb) scylladb#16 0x7fbf45e10a21 (/lib64/libboost_unit_test_framework.so.1.73.0+0x1aa21) scylladb#17 0x7fbf45e20fe0 in boost::execution_monitor::execute(boost::function<int ()> const&) (/lib64/libboost_unit_test_framework.so.1.73.0+0x2afe0) scylladb#18 0x7fbf45e21094 in boost::execution_monitor::vexecute(boost::function<void ()> const&) (/lib64/libboost_unit_test_framework.so.1.73.0+0x2b094) scylladb#19 0x7fbf45e43921 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) (/lib64/libboost_unit_test_framework.so.1.73.0+0x4d921) scylladb#20 0x7fbf45e5eae1 (/lib64/libboost_unit_test_framework.so.1.73.0+0x68ae1) scylladb#21 0x7fbf45e5ed31 (/lib64/libboost_unit_test_framework.so.1.73.0+0x68d31) scylladb#22 0x7fbf45e2e547 in boost::unit_test::framework::run(unsigned long, bool) (/lib64/libboost_unit_test_framework.so.1.73.0+0x38547) scylladb#23 0x7fbf45e43618 in boost::unit_test::unit_test_main(bool (*)(), int, char**) (/lib64/libboost_unit_test_framework.so.1.73.0+0x4d618) scylladb#24 0x44798d in seastar::testing::entry_point(int, char**) ../../src/testing/entry_point.cc:77 scylladb#25 0x4134b5 in main ../../include/seastar/testing/seastar_test.hh:65 scylladb#26 0x7fbf44a1b1e1 in __libc_start_main (/lib64/libc.so.6+0x281e1) scylladb#27 0x4133dd in _start (/home/bhalevy/dev/seastar/build/debug/tests/unit/closeable_test+0x4133dd) ``` Signed-off-by: Benny Halevy <bhalevy@scylladb.com> Message-Id: <20210406100911.12278-1-bhalevy@scylladb.com>

When we enable the sanitizer, we get following error while running iotune: ==86505==ERROR: LeakSanitizer: detected memory leaks Direct leak of 4096 byte(s) in 1 object(s) allocated from: #0 0x5701b8 in aligned_alloc (/home/syuu/seastar.2/build/sanitize/apps/iotune/iotune+0x5701b8) (BuildId: 411f9852d64ed8982d5b33d02489b5932d92b8b7) #1 0x6d0813 in seastar::filesystem_has_good_aio_support(seastar::basic_sstring<char, unsigned int, 15u, true>, bool) /home/syuu/seastar.2/src/core/fsqual.cc:74:16 #2 0x5bcd0d in main::$_0::operator()() const::'lambda'()::operator()() const /home/syuu/seastar.2/apps/iotune/iotune.cc:742:21 #3 0x5bb1f1 in seastar::future<int> seastar::futurize<int>::apply<main::$_0::operator()() const::'lambda'()>(main::$_0::operator()() const::'lambda'()&&, std::tuple<>&&) /home/syuu/seastar.2/include/seastar/core/future.hh:2118:28 #4 0x5bb039 in seastar::futurize<std::invoke_result<main::$_0::operator()() const::'lambda'()>::type>::type seastar::async<main::$_0::operator()() const::'lambda'()>(seastar::thread_attributes, main::$_0::operator()() const::'lambda'()&&)::'lambda'()::operator()() const /home/syuu/seastar.2/include/seastar/core/thread.hh:258:13 #5 0x5bb039 in seastar::noncopyable_function<void ()>::direct_vtable_for<seastar::futurize<std::invoke_result<main::$_0::operator()() const::'lambda'()>::type>::type seastar::async<main::$_0::operator()() const::'lambda'()>(seastar::thread_attributes, main::$_0::operator()() const::'lambda'()&&)::'lambda'()>::call(seastar::noncopyable_function<void ()> const*) /home/syuu/seastar.2/include/seastar/util/noncopyable_function.hh:124:20 #6 0x8e0a77 in seastar::thread_context::main() /home/syuu/seastar.2/src/core/thread.cc:299:9 #7 0x7f30ff8547bf (/lib64/libc.so.6+0x547bf) (BuildId: 85c438f4ff93e21675ff174371c9c583dca00b2c) SUMMARY: AddressSanitizer: 4096 byte(s) leaked in 1 allocation(s). This is because we don't free buffer which allocated at filesystem_has_good_aio_support(), we should free it to avoid such error. And this is needed to test Scylla machine image with debug mode binary, since it tries to run iotune with the sanitizer and fails. Closes scylladb#1284

in main(), we creates an instance of `http_server_control` using new, but we never destroy it. this is identified by ASan ``` ==2190125==ERROR: LeakSanitizer: detected memory leaks Direct leak of 8 byte(s) in 1 object(s) allocated from: #0 0x55e21cf487bd in operator new(unsigned long) /home/kefu/dev/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:86:3 #1 0x55e21cf6cf31 in main::$_0::operator()() const::'lambda'()::operator()() const /home/kefu/dev/seastar/apps/httpd/main.cc:121:27 #2 0x55e21cf6b4cc in int std::__invoke_impl<int, main::$_0::operator()() const::'lambda'()>(std::__invoke_other, main::$_0::operator()() const::'lambda'()&&) /usr/lib/gcc/x86_64-redhat-linux/14/../../../../incl ude/c++/14/bits/invoke.h:61:14 #3 0x55e21cf6b46c in std::__invoke_result<main::$_0::operator()() const::'lambda'()>::type std::__invoke<main::$_0::operator()() const::'lambda'()>(main::$_0::operator()() const::'lambda'()&&) /usr/lib/gcc/x86_ 64-redhat-linux/14/../../../../include/c++/14/bits/invoke.h:96:14 #4 0x55e21cf6b410 in decltype(auto) std::__apply_impl<main::$_0::operator()() const::'lambda'(), std::tuple<>>(main::$_0::operator()() const::'lambda'()&&, std::tuple<>&&, std::integer_sequence<unsigned long, . ..>) /usr/lib/gcc/x86_64-redhat-linux/14/../../../../include/c++/14/tuple:2921:14 #5 0x55e21cf6b3b2 in decltype(auto) std::apply<main::$_0::operator()() const::'lambda'(), std::tuple<>>(main::$_0::operator()() const::'lambda'()&&, std::tuple<>&&) /usr/lib/gcc/x86_64-redhat-linux/14/../../../ ../include/c++/14/tuple:2936:14 #6 0x55e21cf6b283 in seastar::future<int> seastar::futurize<int>::apply<main::$_0::operator()() const::'lambda'()>(main::$_0::operator()() const::'lambda'()&&, std::tuple<>&&) /home/kefu/dev/seastar/include/sea star/core/future.hh:2005:28 #7 0x55e21cf6b043 in seastar::futurize<std::invoke_result<main::$_0::operator()() const::'lambda'()>::type>::type seastar::async<main::$_0::operator()() const::'lambda'()>(seastar::thread_attributes, main::$_0: :operator()() const::'lambda'()&&)::'lambda'()::operator()() const /home/kefu/dev/seastar/include/seastar/core/thread.hh:260:13 #8 0x55e21cf6ae74 in seastar::noncopyable_function<void ()>::direct_vtable_for<seastar::futurize<std::invoke_result<main::$_0::operator()() const::'lambda'()>::type>::type seastar::async<main::$_0::operator()() const::'lambda'()>(seastar::thread_attributes, main::$_0::operator()() const::'lambda'()&&)::'lambda'()>::call(seastar::noncopyable_function<void ()> const*) /home/kefu/dev/seastar/include/seastar/util/noncopyable _function.hh:129:20 #9 0x7f5d757a0fb3 in seastar::noncopyable_function<void ()>::operator()() const /home/kefu/dev/seastar/include/seastar/util/noncopyable_function.hh:215:16 #10 0x7f5d75ef5611 in seastar::thread_context::main() /home/kefu/dev/seastar/src/core/thread.cc:311:9 #11 0x7f5d75ef50eb in seastar::thread_context::s_main(int, int) /home/kefu/dev/seastar/src/core/thread.cc:287:43 #12 0x7f5d72f8a18f (/lib64/libc.so.6+0x5a18f) (BuildId: b098f1c75a76548bb230d8f551eae07a2aeccf06) ``` so, in this change, let's hold it using a smart pointer, so we can destroy it when it leaves the lexical scope. Signed-off-by: Kefu Chai <kefu.chai@scylladb.com> Closes scylladb#2224

Server-side support for UNIX-domain stream sockets

2c364e9

ronen-fr marked this pull request as ready for review August 8, 2019 13:45

mkogan1 requested review from tchaikov, cyx1231st and rzarzynski August 8, 2019 13:48

mkogan1 added the enhancement New feature or request label Aug 8, 2019

mkogan1 changed the title ~~Server-side support for UNIX-domain stream sockets~~ [DNM] Server-side support for UNIX-domain stream sockets Aug 8, 2019

tchaikov reviewed Aug 8, 2019

View reviewed changes

abandoning the thread

80e6d82

ronen-fr force-pushed the wip-crimson-seastar-adding-udsockets branch from 1972858 to 80e6d82 Compare September 5, 2019 08:54

tchaikov closed this Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNM] Server-side support for UNIX-domain stream sockets #7

[DNM] Server-side support for UNIX-domain stream sockets #7

ronen-fr commented Aug 8, 2019 •

edited

Loading

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 11, 2019

tchaikov Aug 8, 2019 •

edited

Loading

ronen-fr Aug 11, 2019

tchaikov Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov Aug 11, 2019

tchaikov Aug 8, 2019

ronen-fr Aug 11, 2019

tchaikov commented Jan 23, 2020

	if (opts.proto == transport::UNIX) {
	if (opts.proto.sa_family == AF_UNIX) {

[DNM] Server-side support for UNIX-domain stream sockets #7

[DNM] Server-side support for UNIX-domain stream sockets #7

Conversation

ronen-fr commented Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Jan 23, 2020

ronen-fr commented Aug 8, 2019 •

edited

Loading

tchaikov Aug 8, 2019 •

edited

Loading