Problem setting an interceptor with priority FIRST_BEFORE_DEFAULT

[JoseCarlosMata]

Hi,

I noticed that there is a potential bug in the latest version if you create an Interceptor with priority FIRST_BEFORE_DEFAULT.

The exception below is thrown cause this code is reached twice even if there is ONLY one Interceptor defined as FIRST_BEFORE_DEFAULT in the method #positionInterceptor()

if (executeFirstSet)
       throw new IllegalStateException("Cannot set more than one AtmosphereInterceptor to be executed first");

The reason is that in the method #configureAtmosphereInterceptor(), the method above is called twice but the executeFirstSet variable is not set to false and the application throws that exception.

Can anybody take a look at it ?

Thanks

[jfarcand]

Can you share a test case?

[JoseCarlosMata]

We were using the version 2.0.6 with an Interceptor defined as below

public class OurAtmosphereInterceptor implements AtmosphereInterceptor, InvokationOrder {

    @Override
    public void configure(final AtmosphereConfig config) {

    }

    @Override
    public Action inspect(final AtmosphereResource resource) {
       //some code
    }

    @Override
    public void postInspect(final AtmosphereResource resource) {

    }

    @Override
    public PRIORITY priority() {
        return FIRST_BEFORE_DEFAULT;
    }
}

When I tried to update the version to 2.1.4, I got the error in AtmosphereFramework#positionInterceptor(InvokationOrder.PRIORITY p, AtmosphereInterceptor c) cause that function is reached twice and the flag "executeFirstSet" is already set as true in the first iteration.

An easy test case could be to define an Interceptor with priority FIRST_BEFORE_DEFAULT and check the logs. You could see the exception thrown.
Let me know if you need more info.

Thanks for taking a look.

[jfarcand]

OK the framework already have a unit test

    @Test
    public void priorityIllegalTest() throws ServletException, IOException {
        framework.addAtmosphereHandler("/*", handler);
        framework.interceptor(new AtmosphereInterceptorAdapter() {

            @Override
            public Action inspect(AtmosphereResource r) {
                return Action.CREATED;
            }

            @Override
            public PRIORITY priority() {
                return InvokationOrder.FIRST_BEFORE_DEFAULT;
            }

            @Override
            public String toString() {
                return "XXX";
            }
        });
        Exception exception = null;
        try {
            framework.interceptor(new AtmosphereInterceptorAdapter() {

                @Override
                public Action inspect(AtmosphereResource r) {
                    return Action.CREATED;
                }

                @Override
                public PRIORITY priority() {
                    return InvokationOrder.FIRST_BEFORE_DEFAULT;
                }

                @Override
                public String toString() {
                    return "XXX";
                }
            });
            fail();
        } catch (Exception ex) {
            exception = ex;
        }
        assertEquals(IllegalStateException.class, exception.getClass());

    }
}

which never failed. Anyway, I will relax the condition and allow more than one interceptor to be defined before the default.

[JoseCarlosMata]

The JUnit is working properly cause it is not following the same flow as we do. It is testing only the interceptor() method.

The problem that I reported arises when the AtmosphereFramework#init(final ServletConfig sc, boolean wrap) is called. At some point, the method AtmosphereFramework#configureAtmosphereInterceptor(ServletConfig sc) is called and this is the one which starts the problem (follow my traces in the method ).

protected void configureAtmosphereInterceptor(ServletConfig sc) {
        String s = sc.getInitParameter(ApplicationConfig.ATMOSPHERE_INTERCEPTORS);
        if (s != null) {
            String[] list = s.split(",");
            for (String a : list) {
                try {
                    AtmosphereInterceptor ai = newClassInstance(AtmosphereInterceptor.class,
                            (Class<AtmosphereInterceptor>) IOUtils
                                    .loadClass(getClass(), a.trim()));
                    //TRACE:  executeFirstSet = false
                    interceptor(ai);
                    //TRACE:  executeFirstSet = true and interceptor A added to the list ´interceptors´.
                } catch (Exception e) {
                    logger.warn("", e);
                }
            }
        }
        logger.info("Installing Default AtmosphereInterceptor");
        s = sc.getInitParameter(ApplicationConfig.DISABLE_ATMOSPHEREINTERCEPTOR);
        if (s == null || !"true".equalsIgnoreCase(s)) {

            s = sc.getInitParameter(ApplicationConfig.DISABLE_ATMOSPHEREINTERCEPTORS);
            if (s != null) {
                excludedInterceptors.addAll(Arrays.asList(s.trim().replace(" ", "").split(",")));
            }

            // We must reposition
            LinkedList<AtmosphereInterceptor> copy = null;
            if (!interceptors.isEmpty()) {
                //TRACE:  a copy is created with interceptor A , but executeFirstSet =true
                copy = new LinkedList(interceptors);
                //TRACE: interceptor A is removed from the list ¨interceptors¨
                interceptors.clear();
            }

            for (Class<? extends AtmosphereInterceptor> a : defaultInterceptors) {
                if (!excludedInterceptors.contains(a.getName())) {
                    interceptors.add(newAInterceptor(a));
                } else {
                    logger.info("Dropping Interceptor {}", a.getName());
                }
            }

            if (copy != null) {
                for (AtmosphereInterceptor i : copy) {
                    //TRACE:  interceptor A is not in the list ¨interceptors¨ but executeFirstSet = true
                    interceptor(i);
                    //TRACE: it crashes !
                }
            }

            logger.info("Set {} to disable them.", ApplicationConfig.DISABLE_ATMOSPHEREINTERCEPTOR, interceptors);
        }
        initInterceptors();
    }

The solution that you committed is going to avoid throwing the exception but it is not going to solve the real problem

Thanks

[jfarcand]

OK...I'm still trying to reproduce the issue.

[jfarcand]

This is now fixed.