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

[Enhancement] InterruptedExceptions should not be ignored in the code.[ProducerImpl] #3830

Closed
1 of 2 tasks
Alonexc opened this issue Apr 27, 2023 · 8 comments · Fixed by #4791
Closed
1 of 2 tasks
Labels
enhancement New feature or request good first issue Issues for first-time contributors

Comments

@Alonexc
Copy link
Contributor

Alonexc commented Apr 27, 2023

Search before asking

  • I had searched in the issues and found no similar issues.

Enhancement Request

image
image
located at:
eventmesh-storage-plugin/eventmesh-storage-rocketmq/src/main/java/org/apache/eventmesh/storage/rocketmq/producer/ProducerImpl.java line 85,99,113,140,
analysis and explanation:
InterruptedExceptions should never be ignored in the code, and simply logging the exception counts in this case as "ignoring". The throwing of the InterruptedException clears the interrupted state of the Thread, so if the exception is not handled properly the information that the thread was interrupted will be lost. Instead, InterruptedExceptions should either be rethrown - immediately or after cleaning up the method’s state - or the thread should be re-interrupted by calling Thread.interrupt() even if this is supposed to be a single-threaded application. Any other course of action risks delaying thread shutdown and loses the information that the thread was interrupted - probably without finishing its task.
Similarly, the ThreadDeath exception should also be propagated. According to its JavaDoc:
If ThreadDeath is caught by a method, it is important that it be rethrown so that the thread actually dies.

Describe the solution you'd like

Just for reference, if you have better modifications, please correct me:
image

Remove this useless assignment; "msg" already holds the assigned value along all execution paths.
image

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@pandaapo
Copy link
Member

pandaapo commented May 3, 2023

About removing supplySysProp(msg, cloudEvent), I have a different opinion. In method supplySysProp(msg, cloudEvent), the Message object seems to be set more value than before calling supplySysProp(msg, cloudEvent). I'm not sure the reason of deleting it.

@Alonexc
Copy link
Contributor Author

Alonexc commented May 4, 2023

image
Instead of removing supplySysProp(msg, cloudEvent), we remove the intermediate quantity msg. The method itself returns the value of this allocation.
image

@pandaapo
Copy link
Member

pandaapo commented May 4, 2023

Ooops. I misunderstood your meaning. Thanks.

@Pil0tXia Pil0tXia added the good first issue Issues for first-time contributors label Jan 19, 2024
@jevinjiang
Copy link
Contributor

Can I work on this issue? kindly assign it to me!

@Pil0tXia
Copy link
Member

@j1803248734 You are welcome to submit a PR.

@jevinjiang
Copy link
Contributor

Completed, pull request submitted, please review.

jevinjiang pushed a commit to jevinjiang/eventmesh that referenced this issue Mar 18, 2024
@jevinjiang
Copy link
Contributor

I observed that the sendOneway, sendAsync, and reply methods did not return the sendResult object. Why does the send method need to return it?

jevinjiang added a commit to jevinjiang/eventmesh that referenced this issue Mar 19, 2024
@pandaapo
Copy link
Member

I observed that the sendOneway, sendAsync, and reply methods did not return the sendResult object. Why does the send method need to return it?

@jevinjiang
Neither sendOneway nor sendAsync need to be concerned with the returned response, unlike the send method.

sendOnewaysendAsync都不需要关注返回的响应,和send方法不同。

jevinjiang added a commit to jevinjiang/eventmesh that referenced this issue Mar 19, 2024
jevinjiang added a commit to jevinjiang/eventmesh that referenced this issue Mar 20, 2024
mxsm pushed a commit that referenced this issue Mar 22, 2024
#4791)

* [ISSUE #3830] InterruptedExceptions should not be ignored in the code.

* [ISSUE #3830] fix code style

* [ISSUE #3830] delete Unneeded throw

* [ISSUE #3830] rethrown InterruptedException

* [ISSUE #3830] InterruptedException return new SendResult

---------

Co-authored-by: JiangShuJu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Issues for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants