Skip to content
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.

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

[C++] Should ThreadPool executor SpawnReal handle exception? #45426

Open
mapleFU opened this issue Feb 5, 2025 · 7 comments
Open

[C++] Should ThreadPool executor SpawnReal handle exception? #45426

mapleFU opened this issue Feb 5, 2025 · 7 comments

Comments

@mapleFU
Copy link
Member

mapleFU commented Feb 5, 2025

Describe the enhancement requested

apache arrow uses arrow::Status to handling error. This works well. However, when using ::arrow::ThreadPool, the internal task will not catch the exception when internal task throw exception, causing the coredump here.

There're many ways for solving this problem:

  1. Make sure the caller doesn't throw any exceptions, and do nothing in arrow libraries
  2. Capture the exception in arrow ThreadPool

Component(s)

C++

@pitrou
Copy link
Member

pitrou commented Feb 5, 2025

Why did you close this as completed @mapleFU ?

@mapleFU mapleFU closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2025
@pitrou
Copy link
Member

pitrou commented Feb 5, 2025

Sorry, my question was more: why close this? Is it not actually a bug?

@mapleFU
Copy link
Member Author

mapleFU commented Feb 5, 2025

Is it not actually a bug?

I'm thinking that:

  1. This might a problem here if internal user throws an exception
  2. However, when I fix this in my case, I found that at least I need handling the error in internal

So I mark this as not planned now

@pitrou
Copy link
Member

pitrou commented Feb 5, 2025

But Apache Arrow is not your personal software, if there is a bug, the issue should remain open.

@pitrou pitrou reopened this Feb 5, 2025
@pitrou
Copy link
Member

pitrou commented Feb 5, 2025

Also, I do think we should indeed handle unexpected exceptions even though we generally expect Arrow routines to be exception-free.

@mapleFU
Copy link
Member Author

mapleFU commented Feb 5, 2025

But Apache Arrow is not your personal software, if there is a bug, the issue should remain open.

Yeah I understand, however when I'm adding this I found handling exception would be much more than "swallow exception in threadpool", at least, the caller ( like BackgroundGenerator ) might deadlocked after exception is thrown but not handled.

@pitrou
Copy link
Member

pitrou commented Feb 5, 2025

I see. That said, I think we can still improve things in ThreadPool somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants