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

[WELD-2580] Ensure InjectionTarget#dispose(Object) method is called in ManagedBean implementation #1916

Merged
merged 3 commits into from May 5, 2019

Conversation

@ljnelson
Copy link
Contributor

commented Apr 25, 2019

[WELD-2580] Ensure InjectionTarget#dispose(Object) method is called i…
…n ManagedBean implementation

Signed-off-by: Laird Nelson <ljnelson@gmail.com>
@WeldJenkins

This comment has been minimized.

Copy link

commented Apr 25, 2019

Can one of the admins verify this patch?

@manovotn

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

ok to test

@manovotn

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Theoretically, this should be alright.
After some poking around code I would say that any Producer that is actually backed by java class (as a bean) is based off BasicInjectionTarget which already overrides dispose() to no-op.
With this in mind, I haven't been able to think of a valid corner case that would be broken by this change. If anyone overrode IT at this point and declared some disposal method, then it has never been called and thus it's a bug in their code.

We'll also need a test for this (basically the content of issue reproducer made into an Arq. test).

@manovotn

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@ljnelson I am fine incorporating this change in next release. Though I would like a test that verifies this functionality. I have no problems adding one but since its your PR I was thinking you maybe want to do that?

@ljnelson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@manovotn Yep; absolutely; just have to get to it. 😄

@ljnelson

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Hi, @manovotn; not sure where to start here; I see tests all over the place. Maybe add something here (https://github.com/weld/core/blob/master/tests-arquillian/src/test/java/org/jboss/weld/tests/injectionTarget/InjectionTargetTest.java)?

Hmm, no; for the test to be interesting it needs to involve a portable extension. I'll wait for your advice on where to add the test.

@manovotn

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Added test for WELD-2580 fix
Signed-off-by: Laird Nelson <ljnelson@gmail.com>
@ljnelson

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Hi, @manovotn; I've added a test. CI failed for some reason, but building weld/core via mvn clean install works for me.

@manovotn

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@ljnelson the failure happens when running in container tests - that means against actual WFLY server.
Can be reproduced by doing export JBOSS_HOME=/path/to/wfly and running tests with -Dincontainer parameter

@manovotn

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@ljnelson pushed a commit that should fix the test, let's see what CI has to say.
I think your original test only worked incidentally because of how Arquillian operates outside of WFLY. Using Instance to obtain and destroy the bean should be a sure shot in all environments.

@manovotn

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Yeah, that worked! :)

@manovotn manovotn merged commit 52778f1 into weld:master May 5, 2019

5 checks passed

Weld CI - JDK 11 Build finished.
Details
Weld CI - JDK 8 Build finished.
Details
Weld Publish CI - JDK 11 Finished
Details
Weld Publish CI - JDK 8 Finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.