This is an automated email from the ASF dual-hosted git repository. wwbmmm pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push: new a3280c62 change UserDataDeleter type from function ptr to std::function (#2431) a3280c62 is described below commit a3280c6263ddd83bdb30b598e13a44e4406d064d Author: youtao guo <coyork...@gmail.com> AuthorDate: Tue Jan 23 13:27:07 2024 +0800 change UserDataDeleter type from function ptr to std::function (#2431) * change UserDataDeleter type from function ptr to std::function * add unittest * check that no copy happen when append the stateful user data * check the destruction is performed correctly --- src/butil/iobuf.cpp | 15 ++++++++----- src/butil/iobuf.h | 5 +++-- src/butil/iobuf_inl.h | 4 ++-- test/iobuf_unittest.cpp | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 10 deletions(-) diff --git a/src/butil/iobuf.cpp b/src/butil/iobuf.cpp index 169e76ce..b4d6ce3b 100644 --- a/src/butil/iobuf.cpp +++ b/src/butil/iobuf.cpp @@ -189,7 +189,7 @@ size_t IOBuf::new_bigview_count() { } const uint16_t IOBUF_BLOCK_FLAGS_USER_DATA = 0x1; -typedef void (*UserDataDeleter)(void*); +using UserDataDeleter = std::function<void(void*)>; struct UserDataExtension { UserDataDeleter deleter; @@ -233,7 +233,8 @@ struct IOBuf::Block { , cap(data_size) , u({0}) , data(data_in) { - get_user_data_extension()->deleter = deleter; + auto ext = new (get_user_data_extension()) UserDataExtension(); + ext->deleter = std::move(deleter); } // Undefined behavior when (flags & IOBUF_BLOCK_FLAGS_USER_DATA) is 0. @@ -267,7 +268,9 @@ struct IOBuf::Block { this->~Block(); iobuf::blockmem_deallocate(this); } else if (flags & IOBUF_BLOCK_FLAGS_USER_DATA) { - get_user_data_extension()->deleter(data); + auto ext = get_user_data_extension(); + ext->deleter(data); + ext->~UserDataExtension(); this->~Block(); free(this); } @@ -395,7 +398,7 @@ IOBuf::Block* share_tls_block() { } // Return one block to TLS. -inline void release_tls_block(IOBuf::Block *b) { +inline void release_tls_block(IOBuf::Block* b) { if (!b) { return; } @@ -1216,7 +1219,7 @@ int IOBuf::appendv(const const_iovec* vec, size_t n) { int IOBuf::append_user_data_with_meta(void* data, size_t size, - void (*deleter)(void*), + std::function<void(void*)> deleter, uint64_t meta) { if (size > 0xFFFFFFFFULL - 100) { LOG(FATAL) << "data_size=" << size << " is too large"; @@ -1233,7 +1236,7 @@ int IOBuf::append_user_data_with_meta(void* data, if (mem == NULL) { return -1; } - IOBuf::Block* b = new (mem) IOBuf::Block((char*)data, size, deleter); + IOBuf::Block* b = new (mem) IOBuf::Block((char*)data, size, std::move(deleter)); b->u.data_meta = meta; const IOBuf::BlockRef r = { 0, b->cap, b }; _move_back_ref(r); diff --git a/src/butil/iobuf.h b/src/butil/iobuf.h index 5f73f4e0..978f9758 100644 --- a/src/butil/iobuf.h +++ b/src/butil/iobuf.h @@ -24,6 +24,7 @@ #include <sys/uio.h> // iovec #include <stdint.h> // uint32_t +#include <functional> #include <string> // std::string #include <ostream> // std::ostream #include <google/protobuf/io/zero_copy_stream.h> // ZeroCopyInputStream @@ -246,11 +247,11 @@ public: // Append the user-data to back side WITHOUT copying. // The user-data can be split and shared by smaller IOBufs and will be // deleted using the deleter func when no IOBuf references it anymore. - int append_user_data(void* data, size_t size, void (*deleter)(void*)); + int append_user_data(void* data, size_t size, std::function<void(void*)> deleter); // Append the user-data to back side WITHOUT copying. // The meta is associated with this piece of user-data. - int append_user_data_with_meta(void* data, size_t size, void (*deleter)(void*), uint64_t meta); + int append_user_data_with_meta(void* data, size_t size, std::function<void(void*)> deleter, uint64_t meta); // Get the data meta of the first byte in this IOBuf. // The meta is specified with append_user_data_with_meta before. diff --git a/src/butil/iobuf_inl.h b/src/butil/iobuf_inl.h index f900b82d..c49896a5 100644 --- a/src/butil/iobuf_inl.h +++ b/src/butil/iobuf_inl.h @@ -37,8 +37,8 @@ inline ssize_t IOBuf::cut_multiple_into_file_descriptor( return pcut_multiple_into_file_descriptor(fd, -1, pieces, count); } -inline int IOBuf::append_user_data(void* data, size_t size, void (*deleter)(void*)) { - return append_user_data_with_meta(data, size, deleter, 0); +inline int IOBuf::append_user_data(void* data, size_t size, std::function<void(void*)> deleter) { + return append_user_data_with_meta(data, size, std::move(deleter), 0); } inline ssize_t IOPortal::append_from_file_descriptor(int fd, size_t max_count) { diff --git a/test/iobuf_unittest.cpp b/test/iobuf_unittest.cpp index e7a6e10d..09dd17da 100644 --- a/test/iobuf_unittest.cpp +++ b/test/iobuf_unittest.cpp @@ -20,6 +20,8 @@ #include <sys/socket.h> // socketpair #include <errno.h> // errno #include <fcntl.h> // O_RDONLY +#include <stdlib.h> +#include <memory> #include <butil/files/temp_file.h> // TempFile #include <butil/containers/flat_map.h> #include <butil/macros.h> @@ -1616,6 +1618,63 @@ TEST_F(IOBufTest, append_user_data_and_consume) { } } +TEST_F(IOBufTest, append_stateful_user_data) { + butil::IOBuf b0; + const int REP = 16; + const size_t len = REP * 256; + std::shared_ptr<char> mem(new char[len], std::default_delete<char[]>()); + std::weak_ptr<char> weaker = mem; + ASSERT_EQ(1, mem.use_count()); + char* data = mem.get(); + for (int i = 0; i < 256; ++i) { + for (int j = 0; j < REP; ++j) { + data[i * REP + j] = (char)i; + } + } + + int incr_upon_destructrion = 0; + + struct Deleter { + Deleter(std::shared_ptr<char> partial, int& incr) : partial(std::move(partial)), incr(&incr), allow_incr(false) {} + ~Deleter() { + if (allow_incr) (*incr)++; + } + Deleter(const Deleter&) { std::abort(); /*if copy, then crash*/ } + Deleter(Deleter&&) noexcept = default; + void operator()(void*) { + partial.reset(); + allow_incr = true; + } + std::shared_ptr<char> partial; + int* incr; + bool allow_incr; + }; + + for (int i = 0; i < 256; i++) { + std::shared_ptr<char> ptr(mem, data + i * REP); + ASSERT_EQ(0, b0.append_user_data(data + i * REP, REP, Deleter{std::move(ptr), incr_upon_destructrion})); + } + ASSERT_EQ(256, b0._ref_num()); + ASSERT_EQ(257, mem.use_count()); + mem.reset(); + butil::IOBuf::BlockRef r = b0._front_ref(); + ASSERT_EQ(1, butil::iobuf::block_shared_count(r.block)); + ASSERT_EQ(len, b0.size()); + std::string out; + ASSERT_EQ(len, b0.cutn(&out, len)); + ASSERT_TRUE(b0.empty()); + ASSERT_TRUE(weaker.expired()); + ASSERT_EQ(256, incr_upon_destructrion); + + ASSERT_EQ(len, out.size()); + // note: cannot memcmp with data which is already free-ed + for (int i = 0; i < 256; ++i) { + for (int j = 0; j < REP; ++j) { + ASSERT_EQ((char)i, out[i * REP + j]); + } + } +} + TEST_F(IOBufTest, append_user_data_and_share) { butil::IOBuf b0; const int REP = 16; --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@brpc.apache.org For additional commands, e-mail: dev-h...@brpc.apache.org