Skip to content

Fix new Sonar smells #2768

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

Merged
merged 6 commits into from
Feb 27, 2019
Merged

Fix new Sonar smells #2768

merged 6 commits into from
Feb 27, 2019

Conversation

artembilan
Copy link
Member

  • Fix NPE around MetricsCaptor in channels
  • Fix potential leaks with MeterFacade in PollableChannel implementations
  • Some refactoring for MessagingMethodInvokerHelper

* Fix some old Sonar smells as well
* Fix Micrometer leaks in the `PollableChannel` when we register
meters, but don't remove them.
@artembilan
Copy link
Member Author

Gary, pay attention, please, what I've done for the MessagingMethodInvokerHelper, especially exception handling.
And with that fix I had to do something for the IntegrationUtils.wrapInHandlingExceptionIfNecessary() to unwrap an IllegalStateException.

Let me know if that is good!

Also my opinion: I don't like how Sonar react to our catch() and re-throws or if...else in catch blocks. Looks like it doesn't see the whole picture and doesn't suggest to some proper solution.
I realized that we can trick it with extra methods where we do the "hard" logic to process an exception 😄

Thanks

@artembilan
Copy link
Member Author

Plus forgot to say: we don't remove MeterFacade in some places, e.g. PollableAmqpChannel.
I will issue a separate PR for 5.1 to back-port that.

* Remove `throws Exception` from `AbstractMessageHandler.destroy()`
if (!(ex instanceof MessagingException) ||
((MessagingException) ex).getFailedMessage() == null) {
runtimeException = new MessageHandlingException(message, text.get(), ex);
runtimeException = new MessageHandlingException(message, text.get(),
(ex instanceof IllegalStateException && ex.getCause() != null) ? ex.getCause() : ex);
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this might be a bit of abuse and we might need to introduce our own exception like MessagingMethodInvokerException to be able to let end-user IllegalStateException to bubble as is.

Of course if you agree with the change to the MessagingMethodInvokerHelper...

@garyrussell garyrussell merged commit d2e974a into spring-projects:master Feb 27, 2019
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.

2 participants