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

[GLUTEN-1199] Avoid throwing exception from destructor of JavaInputStreamAdaptor #1505

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

cambyzju
Copy link
Contributor

What changes were proposed in this pull request?

HINT: Throw an exception in destructor is dangerous and you should never let the exception leave destructors. If there are two exceptions propagating, the program will terminate or yield undefined behavior.

inside function Java_io_glutenproject_vectorized_ShuffleReaderJniWrapper_make

  1. we new object of JavaInputStreamAdaptor
  2. we new object of Reader, and constructor of Reader throw an exception
  3. inside JNI_METHOD_END, we will catch this exception
    But between step2 and step3, destructor of JavaInputStreamAdaptor is called, then we got another exception, the process terminate.

(Fixes: #1199)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

@github-actions
Copy link

#1199

@jackylee-ch
Copy link
Contributor

@FelixYBW
Copy link
Contributor

Thank you for catching this!

Copy link
Contributor

@jinchengchenghh jinchengchenghh left a comment

Choose a reason for hiding this comment

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

Please fix the code style, thanks!

@zhztheplayer zhztheplayer changed the title [GLUTEN-1199] avoid throw exception in destructor [GLUTEN-1199] Avoid throw exception in destructor Apr 26, 2023
@zhztheplayer zhztheplayer changed the title [GLUTEN-1199] Avoid throw exception in destructor [GLUTEN-1199] Avoid throwing exception in destructor Apr 26, 2023
@zhztheplayer zhztheplayer changed the title [GLUTEN-1199] Avoid throwing exception in destructor [GLUTEN-1199] Avoid throwing exception in destructor of "JavaInputStreamAdaptor" Apr 26, 2023
@zhztheplayer zhztheplayer changed the title [GLUTEN-1199] Avoid throwing exception in destructor of "JavaInputStreamAdaptor" [GLUTEN-1199] Avoid throwing exception in destructor of JavaInputStreamAdaptor Apr 26, 2023
zhztheplayer
zhztheplayer previously approved these changes Apr 26, 2023
Copy link
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

Thanks.

And how does this relate to the mentioned memory leak issue? Would you like to explain a bit more about that?

@cambyzju
Copy link
Contributor Author

Please fix the code style, thanks!

done

@cambyzju
Copy link
Contributor Author

Thanks.

And how does this relate to the mentioned memory leak issue? Would you like to explain a bit more about that?

This pr do not relate to memory leak issue. I will try to found it out and fix it next week.

@jackylee-ch
Copy link
Contributor

@zhztheplayer This pr is related to #1187 and it is a better fix to deal with checkException.

@zhztheplayer zhztheplayer changed the title [GLUTEN-1199] Avoid throwing exception in destructor of JavaInputStreamAdaptor [GLUTEN-1199] Avoid throwing exception from destructor of JavaInputStreamAdaptor Apr 26, 2023
@PHILO-HE
Copy link
Contributor

Will merge the patch soon, if there is no more comment.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CORE] Handle the exception when task is interrupted.
6 participants