From 63a76ed11538d004c9f480a7af75dec1631fa9ba Mon Sep 17 00:00:00 2001 From: yiguolei <676222867@qq.com> Date: Fri, 21 Apr 2023 09:13:24 +0800 Subject: [PATCH] [refactor](exceptionsafe) disallow call new method explicitly (#18830) disallow call new method explicitly force to use create_shared or create_unique to use shared ptr placement new is allowed reference https://abseil.io/tips/42 to add factory method to all class. I think we should follow this guide because if throw exception in new method, the program will terminate. --------- Co-authored-by: yiguolei --- be/src/common/factory_creator.h | 55 +++++++++++++++++++ be/src/olap/schema_change.cpp | 11 ++-- be/src/pipeline/exec/data_queue.cpp | 2 +- be/src/pipeline/exec/operator.h | 2 +- be/src/pipeline/pipeline_task.cpp | 2 +- be/src/service/point_query_executor.cpp | 4 +- be/src/vec/common/sort/sorter.h | 2 +- be/src/vec/core/block.cpp | 2 +- be/src/vec/core/block.h | 6 +- be/src/vec/core/block_spill_reader.cpp | 2 +- be/src/vec/exec/scan/pip_scanner_context.h | 9 +-- be/src/vec/exec/scan/scanner_context.cpp | 8 +-- be/src/vec/exec/vrepeat_node.cpp | 2 +- be/src/vec/exec/vsort_node.cpp | 2 +- be/src/vec/runtime/vdata_stream_recvr.cpp | 4 +- be/src/vec/runtime/vdata_stream_recvr.h | 2 +- be/src/vec/sink/vdata_stream_sender.cpp | 2 +- be/src/vec/sink/vresult_file_sink.cpp | 3 +- be/src/vec/sink/vtablet_sink.cpp | 6 +- .../vec/exec/parquet/parquet_reader_test.cpp | 5 +- .../vec/exec/parquet/parquet_thrift_test.cpp | 2 +- be/test/vec/function/function_test_util.cpp | 4 +- 22 files changed, 98 insertions(+), 39 deletions(-) create mode 100644 be/src/common/factory_creator.h diff --git a/be/src/common/factory_creator.h b/be/src/common/factory_creator.h new file mode 100644 index 0000000000..f3ece32685 --- /dev/null +++ b/be/src/common/factory_creator.h @@ -0,0 +1,55 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once +#include + +// All object should inherit from this class, like +// class A { +// DISALLOW_EXPILICT_NEW(A); +// }; +// +// Then the caller could not call new A() any more, has to call A::create_shared(...) +// or A::create_unique(...), then we could make sure all object in our project is shared +// pointer. +// But could call A a(...); + +// A macro to disallow the copy constructor and operator= functions +// This should be used in the private: declarations for a class +// +// Not use template like cowhelper to implements this feature because it has problem +// during inherits +// TODO try to allow make_unique +// +#define ENABLE_FACTORY_CREATOR(TypeName) \ +private: \ + void* operator new(std::size_t size) { return ::operator new(size); } \ + void* operator new[](std::size_t size) { return ::operator new[](size); } \ + \ +public: \ + void* operator new(std::size_t count, void* ptr) { return ::operator new(count, ptr); } \ + void operator delete(void* ptr) noexcept { ::operator delete(ptr); } \ + void operator delete[](void* ptr) noexcept { ::operator delete[](ptr); } \ + void operator delete(void* ptr, void* place) noexcept { ::operator delete(ptr, place); } \ + template \ + static std::shared_ptr create_shared(Args&&... args) { \ + return std::make_shared(std::forward(args)...); \ + } \ + template \ + static std::unique_ptr create_unique(Args&&... args) { \ + return std::unique_ptr(new TypeName(std::forward(args)...)); \ + } diff --git a/be/src/olap/schema_change.cpp b/be/src/olap/schema_change.cpp index 9742b4d095..e4d939b5a0 100644 --- a/be/src/olap/schema_change.cpp +++ b/be/src/olap/schema_change.cpp @@ -459,8 +459,8 @@ Status VSchemaChangeDirectly::_inner_process(RowsetReaderSharedPtr rowset_reader TabletSchemaSPtr base_tablet_schema) { do { auto new_block = - std::make_unique(new_tablet->tablet_schema()->create_block()); - auto ref_block = std::make_unique(base_tablet_schema->create_block()); + vectorized::Block::create_unique(new_tablet->tablet_schema()->create_block()); + auto ref_block = vectorized::Block::create_unique(base_tablet_schema->create_block()); rowset_reader->next_block(ref_block.get()); if (ref_block->rows() == 0) { @@ -531,11 +531,10 @@ Status VSchemaChangeWithSorting::_inner_process(RowsetReaderSharedPtr rowset_rea return Status::OK(); }; - auto new_block = - std::make_unique(new_tablet->tablet_schema()->create_block()); + auto new_block = vectorized::Block::create_unique(new_tablet->tablet_schema()->create_block()); do { - auto ref_block = std::make_unique(base_tablet_schema->create_block()); + auto ref_block = vectorized::Block::create_unique(base_tablet_schema->create_block()); rowset_reader->next_block(ref_block.get()); if (ref_block->rows() == 0) { break; @@ -557,7 +556,7 @@ Status VSchemaChangeWithSorting::_inner_process(RowsetReaderSharedPtr rowset_rea // move unique ptr blocks.push_back( - std::make_unique(new_tablet->tablet_schema()->create_block())); + vectorized::Block::create_unique(new_tablet->tablet_schema()->create_block())); swap(blocks.back(), new_block); } while (true); diff --git a/be/src/pipeline/exec/data_queue.cpp b/be/src/pipeline/exec/data_queue.cpp index 42a1fbf38e..6dbf341972 100644 --- a/be/src/pipeline/exec/data_queue.cpp +++ b/be/src/pipeline/exec/data_queue.cpp @@ -60,7 +60,7 @@ std::unique_ptr DataQueue::get_free_block(int child_idx) { } } - return std::make_unique(); + return vectorized::Block::create_unique(); } void DataQueue::push_free_block(std::unique_ptr block, int child_idx) { diff --git a/be/src/pipeline/exec/operator.h b/be/src/pipeline/exec/operator.h index 15afe49ef6..e48be7c49a 100644 --- a/be/src/pipeline/exec/operator.h +++ b/be/src/pipeline/exec/operator.h @@ -436,7 +436,7 @@ public: StatefulOperator(OperatorBuilderBase* builder, ExecNode* node) : StreamingOperator(builder, node), - _child_block(new vectorized::Block), + _child_block(vectorized::Block::create_unique()), _child_source_state(SourceState::DEPEND_ON_SOURCE) {} virtual ~StatefulOperator() = default; diff --git a/be/src/pipeline/pipeline_task.cpp b/be/src/pipeline/pipeline_task.cpp index 6d69ebfd89..0ef3574a70 100644 --- a/be/src/pipeline/pipeline_task.cpp +++ b/be/src/pipeline/pipeline_task.cpp @@ -98,7 +98,7 @@ Status PipelineTask::prepare(RuntimeState* state) { fmt::format_to(operator_ids_str, "]"); _task_profile->add_info_string("OperatorIds(source2root)", fmt::to_string(operator_ids_str)); - _block.reset(new doris::vectorized::Block()); + _block = doris::vectorized::Block::create_unique(); // We should make sure initial state for task are runnable so that we can do some preparation jobs (e.g. initialize runtime filters). set_state(PipelineTaskState::RUNNABLE); diff --git a/be/src/service/point_query_executor.cpp b/be/src/service/point_query_executor.cpp index ed42c0c9c4..02947f0d37 100644 --- a/be/src/service/point_query_executor.cpp +++ b/be/src/service/point_query_executor.cpp @@ -54,7 +54,7 @@ Status Reusable::init(const TDescriptorTable& t_desc_tbl, const std::vectorset_desc_tbl(_desc_tbl); _block_pool.resize(block_size); for (int i = 0; i < _block_pool.size(); ++i) { - _block_pool[i] = std::make_unique(tuple_desc()->slots(), 10); + _block_pool[i] = vectorized::Block::create_unique(tuple_desc()->slots(), 10); } RETURN_IF_ERROR(vectorized::VExpr::create_expr_trees(_runtime_state->obj_pool(), output_exprs, @@ -69,7 +69,7 @@ Status Reusable::init(const TDescriptorTable& t_desc_tbl, const std::vector Reusable::get_block() { std::lock_guard lock(_block_mutex); if (_block_pool.empty()) { - return std::make_unique(tuple_desc()->slots(), 4); + return vectorized::Block::create_unique(tuple_desc()->slots(), 4); } auto block = std::move(_block_pool.back()); CHECK(block != nullptr); diff --git a/be/src/vec/common/sort/sorter.h b/be/src/vec/common/sort/sorter.h index 3cf45cd537..a7ab99e8cf 100644 --- a/be/src/vec/common/sort/sorter.h +++ b/be/src/vec/common/sort/sorter.h @@ -52,7 +52,7 @@ public: // create_empty_block should ignore invalid slots, unsorted_block // should be same structure with arrival block from child node // since block from child node may ignored these slots - : unsorted_block_(new Block( + : unsorted_block_(Block::create_unique( VectorizedUtils::create_empty_block(row_desc, true /*ignore invalid slot*/))), offset_(offset), limit_(limit), diff --git a/be/src/vec/core/block.cpp b/be/src/vec/core/block.cpp index 638e643d1e..0d5db7063f 100644 --- a/be/src/vec/core/block.cpp +++ b/be/src/vec/core/block.cpp @@ -988,7 +988,7 @@ std::string MutableBlock::dump_data(size_t row_limit) const { } std::unique_ptr Block::create_same_struct_block(size_t size) const { - auto temp_block = std::make_unique(); + auto temp_block = Block::create_unique(); for (const auto& d : data) { auto column = d.type->create_column(); column->resize(size); diff --git a/be/src/vec/core/block.h b/be/src/vec/core/block.h index 26d78566f1..ee1b3debc3 100644 --- a/be/src/vec/core/block.h +++ b/be/src/vec/core/block.h @@ -34,6 +34,7 @@ #include #include +#include "common/factory_creator.h" #include "common/status.h" #include "vec/columns/column.h" #include "vec/columns/column_nullable.h" @@ -67,10 +68,11 @@ namespace vectorized { class MutableBlock; class Block { + ENABLE_FACTORY_CREATOR(Block); + private: using Container = ColumnsWithTypeAndName; using IndexByName = phmap::flat_hash_map; - Container data; IndexByName index_by_name; std::vector row_same_bit; @@ -397,6 +399,8 @@ using BlocksPtr = std::shared_ptr; using BlocksPtrs = std::shared_ptr>; class MutableBlock { + ENABLE_FACTORY_CREATOR(MutableBlock); + private: MutableColumns _columns; DataTypes _data_types; diff --git a/be/src/vec/core/block_spill_reader.cpp b/be/src/vec/core/block_spill_reader.cpp index 629ebc7487..30079360d7 100644 --- a/be/src/vec/core/block_spill_reader.cpp +++ b/be/src/vec/core/block_spill_reader.cpp @@ -134,7 +134,7 @@ Status BlockSpillReader::read(Block* block, bool* eos) { if (!pb_block.ParseFromArray(result.data, result.size)) { return Status::InternalError("Failed to read spilled block"); } - new_block.reset(new Block(pb_block)); + new_block = Block::create_unique(pb_block); } block->swap(*new_block); } else { diff --git a/be/src/vec/exec/scan/pip_scanner_context.h b/be/src/vec/exec/scan/pip_scanner_context.h index d90aa9a911..3637181364 100644 --- a/be/src/vec/exec/scan/pip_scanner_context.h +++ b/be/src/vec/exec/scan/pip_scanner_context.h @@ -137,11 +137,12 @@ public: limit == -1 ? _batch_size : std::min(static_cast(_batch_size), limit); int64_t free_blocks_memory_usage = 0; for (int i = 0; i < _max_queue_size; ++i) { - auto block = std::make_unique(_output_tuple_desc->slots(), - real_block_size, - true /*ignore invalid slots*/); + auto block = vectorized::Block::create_unique(_output_tuple_desc->slots(), + real_block_size, + true /*ignore invalid slots*/); free_blocks_memory_usage += block->allocated_bytes(); - _colocate_mutable_blocks.emplace_back(new vectorized::MutableBlock(block.get())); + _colocate_mutable_blocks.emplace_back( + vectorized::MutableBlock::create_unique(block.get())); _colocate_blocks.emplace_back(std::move(block)); _colocate_block_mutexs.emplace_back(new std::mutex); } diff --git a/be/src/vec/exec/scan/scanner_context.cpp b/be/src/vec/exec/scan/scanner_context.cpp index ae017776f1..aabd95f1e6 100644 --- a/be/src/vec/exec/scan/scanner_context.cpp +++ b/be/src/vec/exec/scan/scanner_context.cpp @@ -103,8 +103,8 @@ Status ScannerContext::init() { // So use _output_tuple_desc; int64_t free_blocks_memory_usage = 0; for (int i = 0; i < pre_alloc_block_count; ++i) { - auto block = std::make_unique( - _output_tuple_desc->slots(), real_block_size, true /*ignore invalid slots*/); + auto block = vectorized::Block::create_unique(_output_tuple_desc->slots(), real_block_size, + true /*ignore invalid slots*/); free_blocks_memory_usage += block->allocated_bytes(); _free_blocks.emplace_back(std::move(block)); } @@ -145,8 +145,8 @@ vectorized::BlockUPtr ScannerContext::get_free_block(bool* has_free_block, *has_free_block = false; COUNTER_UPDATE(_newly_create_free_blocks_num, 1); - return std::make_unique(_real_tuple_desc->slots(), _batch_size, - true /*ignore invalid slots*/); + return vectorized::Block::create_unique(_real_tuple_desc->slots(), _batch_size, + true /*ignore invalid slots*/); } void ScannerContext::return_free_block(std::unique_ptr block) { diff --git a/be/src/vec/exec/vrepeat_node.cpp b/be/src/vec/exec/vrepeat_node.cpp index 5bfa907ccc..8cae519037 100644 --- a/be/src/vec/exec/vrepeat_node.cpp +++ b/be/src/vec/exec/vrepeat_node.cpp @@ -224,7 +224,7 @@ Status VRepeatNode::push(RuntimeState* state, vectorized::Block* input_block, bo DCHECK(!_expr_ctxs.empty()); if (input_block->rows() > 0) { - _intermediate_block.reset(new Block()); + _intermediate_block = Block::create_unique(); for (auto expr : _expr_ctxs) { int result_column_id = -1; diff --git a/be/src/vec/exec/vsort_node.cpp b/be/src/vec/exec/vsort_node.cpp index 5901db2ce6..1923ac27f0 100644 --- a/be/src/vec/exec/vsort_node.cpp +++ b/be/src/vec/exec/vsort_node.cpp @@ -168,7 +168,7 @@ Status VSortNode::open(RuntimeState* state) { // The child has been opened and the sorter created. Sort the input. // The final merge is done on-demand as rows are requested in get_next(). bool eos = false; - std::unique_ptr upstream_block(new Block()); + std::unique_ptr upstream_block = Block::create_unique(); do { RETURN_IF_ERROR_AND_CHECK_SPAN( child(0)->get_next_after_projects( diff --git a/be/src/vec/runtime/vdata_stream_recvr.cpp b/be/src/vec/runtime/vdata_stream_recvr.cpp index 7cc3f9cc71..ec0f4f9d88 100644 --- a/be/src/vec/runtime/vdata_stream_recvr.cpp +++ b/be/src/vec/runtime/vdata_stream_recvr.cpp @@ -147,7 +147,7 @@ void VDataStreamRecvr::SenderQueue::add_block(const PBlock& pblock, int be_numbe int64_t deserialize_time = 0; { SCOPED_RAW_TIMER(&deserialize_time); - block.reset(new Block(pblock)); + block = Block::create_unique(pblock); } auto block_byte_size = block->allocated_bytes(); @@ -186,7 +186,7 @@ void VDataStreamRecvr::SenderQueue::add_block(Block* block, bool use_move) { auto block_bytes_received = block->bytes(); // Has to use unique ptr here, because clone column may failed if allocate memory failed. - BlockUPtr nblock = std::make_unique(block->get_columns_with_type_and_name()); + BlockUPtr nblock = Block::create_unique(block->get_columns_with_type_and_name()); // local exchange should copy the block contented if use move == false if (use_move) { diff --git a/be/src/vec/runtime/vdata_stream_recvr.h b/be/src/vec/runtime/vdata_stream_recvr.h index 17bceeed2a..ae3ebf7acd 100644 --- a/be/src/vec/runtime/vdata_stream_recvr.h +++ b/be/src/vec/runtime/vdata_stream_recvr.h @@ -237,7 +237,7 @@ public: if (_is_cancelled || !block->rows()) { return; } - BlockUPtr nblock = std::make_unique(block->get_columns_with_type_and_name()); + BlockUPtr nblock = Block::create_unique(block->get_columns_with_type_and_name()); // local exchange should copy the block contented if use move == false if (use_move) { diff --git a/be/src/vec/sink/vdata_stream_sender.cpp b/be/src/vec/sink/vdata_stream_sender.cpp index f11364e9ac..b1d27d787b 100644 --- a/be/src/vec/sink/vdata_stream_sender.cpp +++ b/be/src/vec/sink/vdata_stream_sender.cpp @@ -202,7 +202,7 @@ Status Channel::add_rows(Block* block, const std::vector& rows) { if (_mutable_block == nullptr) { SCOPED_CONSUME_MEM_TRACKER(_parent->_mem_tracker.get()); - _mutable_block.reset(new MutableBlock(block->clone_empty())); + _mutable_block = MutableBlock::create_unique(block->clone_empty()); } int row_wait_add = rows.size(); diff --git a/be/src/vec/sink/vresult_file_sink.cpp b/be/src/vec/sink/vresult_file_sink.cpp index f96649712b..785daaaf01 100644 --- a/be/src/vec/sink/vresult_file_sink.cpp +++ b/be/src/vec/sink/vresult_file_sink.cpp @@ -127,7 +127,8 @@ Status VResultFileSink::prepare(RuntimeState* state) { _output_row_descriptor)); } else { // init channel - _output_block.reset(new Block(_output_row_descriptor.tuple_descriptors()[0]->slots(), 1)); + _output_block = + Block::create_unique(_output_row_descriptor.tuple_descriptors()[0]->slots(), 1); _writer.reset(new (std::nothrow) VFileResultWriter( _file_opts.get(), _storage_type, state->fragment_instance_id(), _output_vexpr_ctxs, _profile, nullptr, _output_block.get(), state->return_object_data_as_binary(), diff --git a/be/src/vec/sink/vtablet_sink.cpp b/be/src/vec/sink/vtablet_sink.cpp index 6abc3fb616..7718d000e0 100644 --- a/be/src/vec/sink/vtablet_sink.cpp +++ b/be/src/vec/sink/vtablet_sink.cpp @@ -486,7 +486,7 @@ Status VNodeChannel::add_block(vectorized::Block* block, const Payload* payload, } if (UNLIKELY(!_cur_mutable_block)) { - _cur_mutable_block.reset(new vectorized::MutableBlock(block->clone_empty())); + _cur_mutable_block = vectorized::MutableBlock::create_unique(block->clone_empty()); } std::unique_ptr temp_payload = nullptr; @@ -570,7 +570,7 @@ Status VNodeChannel::add_block(vectorized::Block* block, const Payload* payload, << " jobid:" << std::to_string(_state->load_job_id()) << " loadinfo:" << _load_info; } - _cur_mutable_block.reset(new vectorized::MutableBlock(block->clone_empty())); + _cur_mutable_block = vectorized::MutableBlock::create_unique(block->clone_empty()); _cur_add_block_request.clear_tablet_ids(); } @@ -858,7 +858,7 @@ void VNodeChannel::mark_close() { std::lock_guard l(_pending_batches_lock); if (!_cur_mutable_block) { // add a dummy block - _cur_mutable_block.reset(new vectorized::MutableBlock()); + _cur_mutable_block = vectorized::MutableBlock::create_unique(); } _pending_blocks.emplace(std::move(_cur_mutable_block), _cur_add_block_request); _pending_batches_num++; diff --git a/be/test/vec/exec/parquet/parquet_reader_test.cpp b/be/test/vec/exec/parquet/parquet_reader_test.cpp index e85f976ff0..dc1c28cfa2 100644 --- a/be/test/vec/exec/parquet/parquet_reader_test.cpp +++ b/be/test/vec/exec/parquet/parquet_reader_test.cpp @@ -124,7 +124,7 @@ TEST_F(ParquetReaderTest, normal) { partition_columns; std::unordered_map missing_columns; p_reader->set_fill_columns(partition_columns, missing_columns); - Block* block = new Block(); + BlockUPtr block = Block::create_unique(); for (const auto& slot_desc : tuple_desc->slots()) { auto data_type = vectorized::DataTypeFactory::instance().create_data_type(slot_desc->type(), true); @@ -134,12 +134,11 @@ TEST_F(ParquetReaderTest, normal) { } bool eof = false; size_t read_row = 0; - p_reader->get_next_block(block, &read_row, &eof); + p_reader->get_next_block(block.get(), &read_row, &eof); for (auto& col : block->get_columns_with_type_and_name()) { ASSERT_EQ(col.column->size(), 10); } EXPECT_TRUE(eof); - delete block; delete p_reader; } diff --git a/be/test/vec/exec/parquet/parquet_thrift_test.cpp b/be/test/vec/exec/parquet/parquet_thrift_test.cpp index 498d9cc60d..ded736abe3 100644 --- a/be/test/vec/exec/parquet/parquet_thrift_test.cpp +++ b/be/test/vec/exec/parquet/parquet_thrift_test.cpp @@ -325,7 +325,7 @@ static void create_block(std::unique_ptr& block) { ObjectPool object_pool; doris::TupleDescriptor* tuple_desc = create_tuple_desc(&object_pool, column_descs); auto tuple_slots = tuple_desc->slots(); - block.reset(new vectorized::Block()); + block = vectorized::Block::create_unique(); for (const auto& slot_desc : tuple_slots) { auto data_type = slot_desc->get_data_type_ptr(); MutableColumnPtr data_column = data_type->create_column(); diff --git a/be/test/vec/function/function_test_util.cpp b/be/test/vec/function/function_test_util.cpp index da975a2c84..47905b832c 100644 --- a/be/test/vec/function/function_test_util.cpp +++ b/be/test/vec/function/function_test_util.cpp @@ -290,7 +290,7 @@ Block* create_block_from_inputset(const InputTypeSet& input_types, const InputDa // 1.1 insert data and create block auto row_size = input_set.size(); - std::unique_ptr block(new Block()); + std::unique_ptr block = Block::create_unique(); for (size_t i = 0; i < descs.size(); ++i) { auto& desc = descs[i]; auto column = desc.data_type->create_column(); @@ -357,7 +357,7 @@ Block* process_table_function(TableFunction* fn, Block* input_block, } while (!fn->eos()); } - std::unique_ptr output_block(new Block()); + std::unique_ptr output_block = Block::create_unique(); output_block->insert({std::move(column), descs[0].data_type, descs[0].col_name}); return output_block.release(); }