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

Fix event bus instance leak #2924

Merged
merged 2 commits into from
Apr 7, 2020
Merged

Fix event bus instance leak #2924

merged 2 commits into from
Apr 7, 2020

Conversation

riccardomodanese
Copy link
Contributor

Signed-off-by: riccardomodanese riccardo.modanese@eurotech.com

Brief description of the PR.
Fix a possible leak on Event Bus.
When the Event Bus tries a re-start and, for some reason, the start throws an exception, the new instance will be not properly cleaned.
Added a clean-up

Related Issue
none

Description of the solution adopted
none

Screenshots
none

Any side note on the changes made
none

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #2924 into develop will increase coverage by 0.15%.
The diff coverage is 26.82%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2924      +/-   ##
=============================================
+ Coverage      56.01%   56.17%   +0.15%     
- Complexity      2525     2535      +10     
=============================================
  Files           1040     1040              
  Lines          22578    22597      +19     
  Branches        2037     2039       +2     
=============================================
+ Hits           12648    12694      +46     
+ Misses          8998     8975      -23     
+ Partials         932      928       -4     
Impacted Files Coverage Δ Complexity Δ
.../eclipse/kapua/commons/event/ServiceInspector.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...se/kapua/commons/event/jms/JMSServiceEventBus.java 22.80% <27.50%> (+1.75%) 0.00 <0.00> (ø)
...obDeviceManagementOperationManagerServiceImpl.java 73.17% <0.00%> (-6.10%) 13.00% <0.00%> (-1.00%)
.../jbatch/persistence/JPAPersistenceManagerImpl.java 31.60% <0.00%> (+1.03%) 30.00% <0.00%> (ø%)
...plugin/authentication/UserAuthenticationLogic.java 76.78% <0.00%> (+1.78%) 0.00% <0.00%> (ø%)
...apua/job/engine/jbatch/JobEngineServiceJbatch.java 51.85% <0.00%> (+1.85%) 8.00% <0.00%> (ø%)
...atch/persistence/jpa/JpaExecutionInstanceData.java 90.24% <0.00%> (+2.43%) 23.00% <0.00%> (+1.00%)
...a/org/eclipse/kapua/transport/mqtt/MqttClient.java 62.19% <0.00%> (+6.09%) 14.00% <0.00%> (ø%)
.../DeviceManagementOperationRegistryServiceImpl.java 73.21% <0.00%> (+7.14%) 16.00% <0.00%> (+4.00%)
...ransport/mqtt/pooling/PooledMqttClientFactory.java 71.42% <0.00%> (+9.52%) 7.00% <0.00%> (ø%)
... and 6 more

@Coduz Coduz added the Bug This is a bug or an unexpected behaviour. Fix it! label Apr 1, 2020
Copy link
Contributor

@stefanomorson stefanomorson left a comment

Choose a reason for hiding this comment

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

Moving the listener to its own class is ok but stopping the retries is probably the most important change here. Agree with these two.
I'm a bit concerned that there could be a race condition on the restart() method due to the fact that while the restart is in execution a new exception coming from the old instance or the new instance may trigger a new restart process. However, if restart is going to be synchronized, we have to make sure that the onException() method originating it must not block the qpid library underneath.

Signed-off-by: riccardomodanese <riccardo.modanese@eurotech.com>
Signed-off-by: riccardomodanese <riccardo.modanese@eurotech.com>
@Coduz Coduz merged commit c1dcd2c into develop Apr 7, 2020
@Coduz Coduz changed the title fix event bus instance leak Fix event bus instance leak Apr 7, 2020
@Coduz Coduz deleted the fix-eventBus branch April 7, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This is a bug or an unexpected behaviour. Fix it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants