Skip to content

[onert] Ensure proper pipeline cleanup on exceptions and shutdown #16379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hseok-oh merged 1 commit into from
Feb 23, 2026
+10 −10
Merged

[onert] Ensure proper pipeline cleanup on exceptions and shutdown #16379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions runtime/onert/backend/trix/ops/BulkPipelineLayer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ void BulkPipelineLayer::run()
}
catch (const std::exception &e)
{
_pipeline_manager->shutdown();
Copy link
Copy Markdown
Contributor Author

@batcheu batcheu Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From now on, any of exceptions including 'Asynchronous buffer filling' and 'Model execution' will call shutdown() explicitly to release resources properly.

Copy link
Copy Markdown
Contributor

@ragmani ragmani Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if BulkPipelineManager::shutdown() is always guaranteed not to throw exceptions. However, the current pipeline are already complex, and I'm don't know how to resolve this issue. So, I want to ignore this and move on to the next stage.

Copy link
Copy Markdown
Contributor Author

@batcheu batcheu Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no case of throwing additional exception in BulkPipelineManager::shutdown(). And even if an exception occurrs, it will abandon the previous one and proceed to the next one. 😊

std::cerr << "BulkPipelineLayer execution failed: " << e.what() << std::endl;
throw;
}
Expand Down
5 changes: 0 additions & 5 deletions runtime/onert/backend/trix/ops/BulkPipelineManager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,6 @@ bool BulkPipelineManager::initialize()

void BulkPipelineManager::shutdown()
{
if (!_initialized.load())
{
return;
}

_initialized = false;

// Wait until all executions are finished
Expand Down
5 changes: 0 additions & 5 deletions runtime/onert/backend/trix/ops/BulkPipelineModel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,6 @@ bool BulkPipelineModel::prepare()

void BulkPipelineModel::release()
{
if (!_prepared.load())
{
return;
}

// Cancel a asynchronous job
if (_async_fill_future.valid())
{
Expand Down
9 changes: 9 additions & 0 deletions runtime/onert/backend/trix/ops/test/BulkPipelineManager.test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class BulkPipelineManagerTest : public ::testing::Test
void TearDown() override {}

std::unique_ptr<BulkPipelineManager> manager;
const int nr_models = 1;
Copy link
Copy Markdown
Contributor Author

@batcheu batcheu Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed to update this line in previous PR ;(

-- const int nr_models = 2;
++ const int nr_models = 1;

};

TEST_F(BulkPipelineManagerTest, test_initilize)
Expand All @@ -71,9 +72,17 @@ TEST_F(BulkPipelineManagerTest, test_initilize)

TEST_F(BulkPipelineManagerTest, test_shutdown)
{
int nr_fclose_calls = 0;
EXPECT_TRUE(manager->initialize());
// This hook will checking the number of fclose() calls
MockSyscallsManager::getInstance().setFcloseHook([&nr_fclose_calls](FILE *) -> int {
nr_fclose_calls++;
return 0;
});
manager->shutdown();
EXPECT_FALSE(manager->isInitialized());
// fclose() should be called as the same number of models
EXPECT_EQ(nr_fclose_calls, nr_models);
Comment on lines +75 to +85
Copy link
Copy Markdown
Contributor Author

@batcheu batcheu Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is difficult to test the manipulated failure of BulkPipelineBuffer:startAsyncBufferFill(). 😢
However, it is possible to indirectly verify whether resources are released when an issue arises by confirming whether the resources of BulkPipelineBuffer are released upon calling BufferPipelineManager::shutdown().

}

TEST_F(BulkPipelineManagerTest, test_execute)
Expand Down