JERSEY-2291: Enable HK2 Immediate scope to support HK2 @Immediate services.

[jordmoz]

https://java.net/jira/browse/JERSEY-2291

Hopefully addresses the above issue.

[jerseyrobot]

Can one of the admins verify this patch?

[AdamLindenthal]

Hi Jord,
many thanks for contributing.
Before we can review your pull request, we need you to sign Oracle Contributor Agreement.

More information about contributing to Jersey: https://jersey.java.net/scm.html#/Submitting_Patches_and_Contribute_Code

Thanks,
Adam

[AdamLindenthal]

Jenkins, please test this patch.

[jordmoz]

Thanks, I just submitted my OCA today.

[mpotociar]

Hi Jord,

In order to accept your patch, it needs to come up with at least one test that verifies the enhancement works as expected (your case) or that contains a reproducer for the fixed bug (in case of a bug fix patch).

Cheers,
Marek

[jordmoz]

I have added a test case which verifies the added functionality. Please take another look.

[jordmoz]

I think the test failure from before may have been spurious. I just synced my local fork with master and re-ran the failed test and it passed. Please take another look.

[mgajdos]

Missing copyright header. Please use a header e.g. from here: https://github.com/jersey/jersey/blob/master/core-common/src/test/java/org/glassfish/jersey/process/internal/ExecutorProvidersTest.java

[jordmoz]

Done.

[mgajdos]

Overall looks good. Please, fix 2 minor issues and we'll merge this pull request.

[jordmoz]

I have addressed all review comments. Please take another look.

[mgajdos]

Jenkins, test this patch.

[mgajdos]

There is one failing test - seems it's caused by your change (verified on my machine). I'll take a look at it tomorrow.

[jordmoz]

I am unable to fail that test. I've synced up to jersey head and am doing
'mvn test' in the core-client directory. What am I doing wrong?

On Wed, Jun 24, 2015 at 11:41 AM, Michal Gajdos notifications@github.com
wrote:

There is one failing test - seems it's caused by your change (verified on
my machine). I'll take a look at it tomorrow.

—
Reply to this email directly or view it on GitHub
https://github.com/jersey/jersey/pull/173#issuecomment-114976088.

[mgajdos]

mvn clean install -pl core-client -am does it for me. Even on master branch – this line

ServiceLocatorUtilities.enableImmediateScope(result);

causes that ClientRuntimes are not released. Not sure why at the moment.

[mgajdos]

The problem is in the test itself. ClientRuntimes are released but not as fast as without the Immediate scope. Please, change the ShutdownHookLeakTest to this:

public class ShutdownHookLeakTest {

    private static final int ITERATIONS = 4000;

    @Test
    public void testShutdownHookDoesNotLeak() throws Exception {
        final Client client = ClientBuilder.newClient();
        final WebTarget target = client.target("http://example.com");

        final Collection shutdownHooks = getShutdownHooks(client);

        for (int i = 0; i < ITERATIONS; i++) {
            // Create/Initialize client runtime.
            target.property("Washington", "Irving")
                    .request()
                    .property("how", "now")
                    .buildGet()
                    .property("Irving", "Washington");
        }

        assertThat(
                "shutdown hook deque size should not copy number of property invocation",
                // 66 % seems like a reasonable threshold for this test to keep it stable
                shutdownHooks.size(), is(lessThan(ITERATIONS * 2 / 3)));
    }

    private Collection getShutdownHooks(final Client client) throws NoSuchFieldException, IllegalAccessException {
        final JerseyClient jerseyClient = (JerseyClient) client;
        final Field shutdownHooksField = JerseyClient.class.getDeclaredField("shutdownHooks");
        shutdownHooksField.setAccessible(true);
        return (Collection) shutdownHooksField.get(jerseyClient);
    }
}

[AdamLindenthal]

Hi jordmoz, sorry for the delay.
I was actually working on that issue without knowing, that there is a pull request for the same thing, it wasn't linked back here from JIRA.
And surprisingly :) I came to the same result - the same failing test and the same solution - to increase the number of iterations and increase the "instance-tollerance" from 1/2 to 2/3 (which has already been done inbetween).
So, would you please increase the constant to let's say 5000 and commit? You will also probably need to rebase, but than we can finally merge and close the PR.

Thanks,
Adam

[jordmoz]

Yeah, it seemed like an easy fix :) Writing the test was fun; I didn't know HK2 used a separate thread for the Immediate scoped object instantiations, so my tests kept failing.

Wow, 5000 seems high. Right now it's ... 500? And I think it fails saying it was at 1000. (If memory serves.) So we're increasing it 10x ... I'm fine with this if you are, but perhaps a lower limit would also be acceptable? I haven't touched this in a while, so maybe the limit is different now. I'll work on this again and fix it.

[Update: I misunderstood the difference between iterations and the limit. 5000 is fine. Above test code references 4000 though and that's what I ended up using.]

[jordmoz]

I took the above test code from @mgajdos and the test now passes. I also rebased everything to current jersey master.

[AdamLindenthal]

Hi jordmoz,

right now it was 1000, not sure what the number was when Michal suggested 4000. I'll retrigger the verification build and we will see. The thing is, of course for testing it would be great to have lower number of iterations, but for "some" reason (the reason being, that the test is not really deterministic), it kept failing when I tried the same change as you are suggesting locally.

At least on my JVM/configuration/environment, seems like the lower amount of iterations did not "force" the GC to work. Of course the solution is not systematic, is just to make it work and not force you to fix test not related to your patch.

Regards,
Adam

[AdamLindenthal]

Jenkins, please test this patch.

[jordmoz]

Test still fails :(

shutdown hook deque size should not copy number of property invocation
Expected: is a value less than <2666>
     but: <4000> was greater than <2666>
Stacktrace

java.lang.AssertionError: shutdown hook deque size should not copy number of property invocation
Expected: is a value less than <2666>
     but: <4000> was greater than <2666>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:956)
    at org.glassfish.jersey.client.ShutdownHookLeakTest.testShutdownHookDoesNotLeak(ShutdownHookLeakTest.java:82)

[jordmoz]

It does pass locally with

mvn clean install -pl core-client -am

which was failing before.

[AdamLindenthal]

I see. The test needs to be changed. The patch definitely influences the test pass rate, but apparently, as it does not fail everytime (and sometime it fails with the number of instances higher than the treshold, but lower than the number of iterations), I guess we can assume that it does not cause a new leak.

For now, I'll just try to re-run the build, in the meantime I'll try to think of what to do with the test.

Cheers,
Adam

[AdamLindenthal]

Jenkins, please re-test this patch.

[AdamLindenthal]

Jenkins, please re-test this patch.

[jordmoz]

Frustrating. :(

[AdamLindenthal]

Indeed. The test is not really deterministic, however I didn't find with any better idea how to test the change it tests.

Just spent some time experimenting with it and came up with one more desperate try, counting instances not enqueued to the reference queue and/or references, that still point to a live object. This reduces the total number, at least on my environment. Hard to predict, how it will behave on Jenkins though.

Could you please try to change the assert to the following code:

        System.gc();

        int notEnqueued = 0;
        int notNull = 0;
        for (Object o : shutdownHooks) {
            if (((WeakReference<JerseyClient.ShutdownHook>) o).get() != null) {
                notNull++;
            }
            if (!((WeakReference<JerseyClient.ShutdownHook>) o).isEnqueued()) {
                notEnqueued++;
            }
        }

        assertThat(
                "Non-null shutdown hook references count should not copy number of property invocation",
                // 66 % seems like a reasonable threshold for this test to keep it stable
                notNull, is(lessThan(THRESHOLD)));

        assertThat(
                "Shutdown hook references count not enqueued in the ReferenceQueue should not copy number of property invocation",
                // 66 % seems like a reasonable threshold for this test to keep it stable
                notEnqueued, is(lessThan(THRESHOLD)));

I hope that this will make the test more stable, but without trying it on Jenkins, we will never know...

Regards,
Adam

[jordmoz]

I rebased onto jersey master and put in your patch. Please try jenkins again.

[jordmoz]

Hrm I seem to be missing a client.close(), which is present in the master version. Is that not needed? I think a previous patch may have taken it out. The test does pass for me when running:

mvn clean install -pl core-client -am

[AdamLindenthal]

Jenkins, please test this patch.

[AdamLindenthal]

Thanks. Well, I guess the test is not necessary, but I would think it will be merged anyway. I hope this time the test passes also on Jenkins, the previous version did pass on my environment as well, but the VM on Jenkins seems to be more lazy about GCing.

Adam

[AdamLindenthal]

Wow, nice! :-)

[AdamLindenthal]

Thanks for the contribution and sorry for hassle with the failing test.

Cheers,
Adam

[jordmoz]

Crazy. Thanks for all your help!

[stepanv]

Hi, I'm sorry but we have to revert your change. The reason is that HK2 does not work as expected and a memory leak can occur under certain circumstances. Tracked here https://java.net/jira/browse/HK2-280.

[jordmoz]

Argh. Thanks for the update.

[stepanv]

Now, when https://java.net/jira/browse/HK2-280 is solved. It might be possible to make it work. If anyone is interested, don't hesitate to get into it. I gave it a quick shoot but without success (https://java.net/jira/browse/JERSEY-2291?focusedCommentId=390642&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-390642 )
Don't forget to also test jersey client when immediate scope services are enabled - we don't want extra thread for every client in a jvm. (See https://github.com/jersey/jersey/blob/master/core-client/src/test/java/org/glassfish/jersey/client/ShutdownHookLeakTest.java which I just enabled (was disabled because of this PR)).