Removes senseless restrictions in ServletContext

[arjantijms]

ServletContext has a number of methods that are restricted if the context has been passed into any receiver that has been dynamically added.

E.g. in GlassFish:

   /**
     * Gets the major version of the Servlet specification that the
     * application represented by this ServletContext is based on.
     */
    @Override
    public int getEffectiveMajorVersion() {
        if (isRestricted) {
            throw new UnsupportedOperationException(
                    rb.getString(LogFacade.UNSUPPORTED_OPERATION_EXCEPTION));
        }
        return context.getEffectiveMajorVersion();
    }

Or Piranha:

   /**
     * {@return the effective major version}
     */
    @Override
    public int getEffectiveMajorVersion() {
        checkTainted();

        if (effectiveMajorVersion == -1) {
            return getMajorVersion();
        }

        return effectiveMajorVersion;
    }

    private void checkTainted() {
        if (tainted) {
            throw new UnsupportedOperationException("ServletContext is in tainted mode (as required by spec).");
        }
    }

A restricted or tainted mode for no reason other than the spec requiring it (also for no good reason) is best removed. This has been discussed on the mailing list before, but no issue was created for this.

I propose to fully remove the artificial restricted/tainted mode that the ServletContext must be put in when it's passed into dynamically added artefacts.

[gregw]

I agree that this appears a senseless restriction on getEffectiveMajorVersion and getEffectiveMinorVersion, so +1 on removing the restriiction from them.

But I'm not so sure the restrictions are that senseless on other methods. I believe they are there to stop the situation that a dynamic component adds a dynamic component that addes a dyanmic component... Which can be a good simple protection against concurrent modification issues during initialization. Such issues can be moderately simply fixed, but I think the TCK would need to have additional tests added to check that containers do it correctly.... and also we'd need to specificy things like if a ContextListener is dynamically added by a ContextListener, then is it's contextInitialized method called or not, and in what order.

If we were starting from scratch, I think I could be convinced to not have this protection (and the apps will soon detect if they have infinite component add loops). But at this stage, specially with time short between now and EE9, I'm not sure the cost/benefit justifies this change at this stage.

How about just removing it from things like getters (looks like a cut/paste error to me), and we can review the adders separately.

[arjantijms]

How about just removing it from things like getters (looks like a cut/paste error to me), and we can review the adders separately.

That's a start for sure.

I vaguely remember something about it being all new methods after declarative listeners were introduced. Something about programmatic added listeners at the time internally coming from the .tld in JSP, and those were from the old version. It was some kind of nudge to move people away from .tld and into using declarative in web.xml or using annotations.

[stuartwdouglas]

+1 to removing it from the getters.

Removing it from the others is not as simple as just deleting the JavaDoc, as from memory there are a lot of TCK tests around this behavior. They would need to be updated to make sure that the new behavior works correctly.

[mnriem]

At minimum remove it from the getters/setters, but preferably strengthen the JavaDoc around dispatching of the event the listener reacts to and disallow adding of that particular listener type and for the initializer once any initializer is called no other initializers can be added.