Replaced Context implement Closeable by AutoCloseable

[rPraml]

With #864 Context was extended to implemet Closeable. But this does not meet the interface contract.

Javadoc of AutoCloseable.close():

Note that unlike the close method of Closeable, this close method is not required to be idempotent. In other words, calling this close method more than once may have some visible side effect, unlike Closeable.close which is required to have no effect if called more than once. However, implementers of this interface are strongly encouraged to make their close methods idempotent.

In other words, when "Context implements Closeable"

Context cx = Context.enter();
cx.close();
cx.close(); // will fail

must not fail.

If only Autocloseable is implemented, it is allowed (though not recommended)

[aardvark179]

This seems objectively worse. If you are using Context via a construct like

try (Context cx = something()) {
     //Do wrok
}

Then probably something() should check that we aren't reentering an existing context., or it should return a proxy that can detect that it is only closed once.

Adhering to a spec in a way that will just cause people to shoot themselves in the foot isn't an improvement.

In other words I would say that you should not change the type of Context such that

Context cx = Context.enter();
cx.close();
cx.close(); // will fail

s allowed. You should work out how to fix things so that code does not fail.

[rPraml]

This seems objectively worse...

I completely agree with you
Unfortunately the "mistake" was made with #864 - I tried to change the code to be at least compliant to the spec.

...should return a proxy

Yes, that would be the right solution. But it would create new problems:

• this adds overhead
• we have either to return a CloseableContext extends Context.(then you cannot cast if you have a custom context implementation)
• or query the context factory for a new context object when nesting occurs

I think we should first vote on which solution we prefer... I hope I haven't opened Pandora's box here

[aardvark179]

Well, changing from Closable to AutoClosable is technically a breaking change, so I fear the box is well and truly open.

The issue we have is that we have two patterns (enter and exit, and get and close) which just don’t quite match.

[gbrail]

yes, enter and exit don't quite match, that's part of the problem. And we do have a problem with tests that don't close context, which is why the ":tests:test" task does a fork and takes forever to run.

What if we change "close" so that is is idempotent, but add an assertion so that people don't abuse it in tests. Does that help any?

IMO a lot of the "Kit.codeBug()" is in there because that code was literally written before Java had "assert!"

[rPraml]

What if we change "close" so that is is idempotent, but add an assertion so that people don't abuse it in tests. Does that help any?

I'm unsure, if that will work.

Context cx1 = Context.enter();
   Context cx2 = Context.enter();
   cx2.someCode
   cx2.close();
   cx2.close();
cx1.someCode // will fail, because all contexts are closed
cx1.close();  // will adittionally throw a codebug-exception

You cannot make it idempotent, as long as Context.enter() returns the same object.

One possible migration-path I see:

• we make the Context.close() deprecated and maybe remove it some days
• we make a Context.open() (or Context.start()) that calls Context.enter() and wraps the context in a CloseableContext
• CloseableContext might delegate some/all methods to the real context, but it cannot be an instanceOf Context

class CloseableContext implements Closeable {
    private final Context theRealContext;
    private boolean closed;

    void close() {
         if (!closed) {
             closed = true;
             theRealContext.exit();
         }
    }
}

or should we refactor everything, so that Closeable becomes an interface and we can return java-proxies (what about performance... and does this work for android?)

[rPraml]

How do we proceed from here? I realize that behavior of close is far from optimal.

You should work out how to fix things so that code does not fail.

@aardvark179 I basically agree with you here. But I wasn't the one who did it wrong (that's not meant as an excuse or a blame). I simply tried to make this PR at least as compliant as the Closeable/Autocloseable interface requires, even if the behaviour is error prone (but would be spec-conform until we have a better solution)

If we have a coordinated plan on how to do it right, I am also happy to invest work here.

[gbrail]

Yes, this is my fault for assuming that most Rhino code entered the context once and should exit it once. I didn't understand the weirdness of Context in that it allows you to enter multiple times.

What if we just changed close so that it WAS idempotent? I.e. this won't fail:

Context cx = Context.enter();
try (Context cx2 = Context.enter()) { }
Context.exit();
Context.exit();

It still lets people use try-with-resources and also doesn't break their code.

I see more problems with code NOT exiting contexts than exiting it multiple times.

[rPraml]

I've just thought again about how we can solve the problem and I have to say, it's anything but simple.

As long as Context.enter() returns the same instance when called nested, we won't be able to do this.
How should we distinguish between

Context cx1 = Context.enter();
Context cx2 = Context.enter();
cx2.close(); // should switch to cx1
cx2.close(); // should do nothing (but cx1 == cx2)
cx1.close(); // closes the outer context

So we must either

• remove the implements Closeable and add a Context.open() that returns a ContextHolder implements Closeable (not an option)
• forbid nesting (not an option)
• create a copy of current Context with all fields (seems to be the easiest task. See PoC: Context.close() is idempotent by returning a copy on enter() #2011 - but cumbersome)
• change Context to become an interface and return some delegates (makes IMHO sense, but means a huge API-change, see PoC: Context.close() is idempotent because Context is an interface now #2012)
• Accept that closing is not idempotent and change Closeable to AutoCloseable. We document this clearly, and anyone who uses close incorrectly is referred to the documentation.

what do you think

[aardvark179]

I'm not sure I like either of those PoCs, but that is partly because I'm not convinced they are tackling the actual problem.

Copying the context every time is also not a great option because you are likely changing the semantics of a lot of stuff since there is a bunch of state which isn't final only every context object and you may affecting that in ways you really didn't intend, for example what happens if my copies of contexts disagree about whether they're being debugged?

Changing Context to an interface doesn't really solve our problems because it breaks the API and people have extended context (for example here and here on a very quick GH search, I'm sure there are others), but in your PoC I also don't understand what the purpose of the impl() method is. I think I'd be okay with forcing subclasses to be changes a little bit, but it would be good if we can avoid too much breakage, so I'd strongly encourage an abstract class instead of an interface.

I like the idea of delegates, and I think that is a thread worth pulling on.

I'm also unsure why you say that removing Closable from Context isn't an option. We're going to break something here, and so we should probably consider everything that will help, and Context is returned in many places where it might be easy to think you should close it, but really shouldn't.

So, let's take a step back and think about the problem(s) we really want to solve.

1. Context is closable, but that may be a bad idea because we have quite a few APIs that return a current context which shouldn't be closed.
2. enter and exit exist, and are manipulating a enter count variable. exit is especially problematic because it's a static function so it's hard to detect when it is being misused.
3. Entry and exit of contexts can be nested. What that should mean in practice may not be entirely clear, but it's definitely needed in code which might not know if it is inside a context.
4. We would like to have a way to detect when things have gone wrong (i.e. we should be able to detect a double exit/close, and possibly report it, and it would be great if we could report cases where a context wasn't exited).

Also, we use contexts a lot, both passing them round explicitly and through getting the current context from our thread local, so anything we do has to be extremely fast and make as few semantic changes as possible.

Maybe something to try would be this

1. Start by making exit an instance method instead of a static one. It won't solve our problems but it does require changing things in about 50 places in our code base, and likely a smaller number in downstream users code. Depending on how we want to handle this we might need to keep the static but mark it as deprecated for removal.
2. Change Context.enter to return an object that delegates to a base Context. That might require some refactoring, maybe making Context an abstract super class of the real context implementation, and our delegate another subclass.

I think those two changes might give us enough tools to start fixing this properly. The delegate returned by enter could keep track of whether it has been closed (making exit or close properly idempotent), and we could reasonably offer a debug setting to track the source of enter calls to help find and debug any missing exits. You can also make the result of calling enter an auto-closable thing, and take that property away from the main Context class.

These would be API changes, but I don't think there is any good way to fix this without that, so this may be an exercise in finding the least breaking change and working out how to stage things so problems can be found.

[rPraml]

I'm not sure I like either of those PoCs, but that is partly because I'm not convinced they are tackling the actual problem.

Yes, perhaps we should revisit and clarify what the exact problem is. (see below)

Copying the context every time is also not a great option...

hmm. We need to return a different instance on Context.enter to implement Closeable correctly. So we need either to copy that context or return some wrapper with a delegate

... changing the semantics ...

Perhaps we also need to clearly define the intended semantics, rather than simply adopting an incorrect semantics dictated by the historical/faulty implementation.

Let me make an example: We have a java app, that executes some javascript code. This code some java code and this java code calls some other javascript code.

class App {
    void execJs() {
       try (Context cx = Context.enter()) {
           cx.setLanguageVersion(Context.VERSION_ES6);
           cx.evaluateString(..., "Util.someLegacyFunc()", ...);
           cx.evaluateString(... more ES6 code ..) // code will be executed in language version 1.2
       }
    }
}
class Util {
   static void someLegacyFunc() {
       try (Context cx = Context.enter()) {
           cx.setLanguageVersion(Context.VERSION_1_2);
           cx.evaluateString(..., "some really old script code", ...);
       }
   }
}

I think this example shows that the current implementation of nested contexts is error-prone, and that it might even be necessary to always return a new context. There are also methods like ContextFactory.enterContext(cx)/call, which just re-use an existing context.

Changing Context to an interface doesn't really solve our problems because it breaks the API and people have extended context

I fear that anyone who has extended Context will need to modify their code. However, I’d like to avoid breaking the API for any methods that simply use the context. Therefore, an abstract class would likely be preferable to an interface, since otherwise dependent libraries would need to be recompiled (interfaces require invokeInterface instead of invokeVirtual).

I also don't understand what the purpose of the impl() method is.

I've used this as a shortcut, because there are many places in the code base, where package-private members are directly accessed. Would be easier, if we use getter/setter for this. I also think that people might need such a method to get their extended context, if a delegate-wrapper was returned

Context is returned in many places where it might be easy to think you should close it, but really shouldn't.

1. Context is closable, but that may be a bad idea because we have quite a few APIs that return a current context which shouldn't be closed.

Normally, you should not close things that you do not own. However, some IDEs do suggest closing such returned values.

Maybe, we can deprecate the close or remove completely and use a new API (e.g. try (CloseableContext ch = Context.open())

2. `enter` and `exit` exist, and are manipulating a enter count variable. `exit` is especially problematic because it's a static function so it's hard to detect when it is being misused.

I would say, each enter should return a new context and exit should restore the old context. See "App" example above.

3. Entry and exit of contexts can be nested. What that should mean in practice may not be entirely clear, but it's definitely needed in code which might not know if it is inside a context.

Nesting is one of the major problems I see in the whole discussion. We currently just count up a value. Maybe I can try an approach with optional nesting. (so reduce nesting as much as possible.

4. We would like to have a way to detect when things have gone wrong (i.e. we should be able to detect a double exit/close, and possibly report it, and it would be great if we could report cases where a context wasn't exited).

Isn't a double close exactly, what we want...

I’ll be away for a few days, but I’ll give it some more thought. Thanks for your detailed feedback.

Roland

[rPraml]

I couldn’t let this go, so in #2014 I tried out an approach to see whether we could completely avoid nesting.

The idea is to execute everything using call(ContextAction).
call only creates a new context if none already exists.
The context in call is sealed. If you want to configure it during its initial creation, there could be call(config, action) or callExplicit, which would always create a new context.

This would effectively allow us to remove the nestedCounter and we could add a closed flag to make it idempotent.
I don't know, if there are use cases, where you want to re-open a context

I’m not yet sure what I would do with enter and exit. Possibly deprecate them and migrate everything to call, which would make Closeable obsolete
(If everything were migrated to call, we could later use scoped values instead of ThreadLocals.)

[gbrail]

I still haven't heard a solution to the problem that really makes sense, and I'm grappling with this too.

What if we:

1. Un-did making Context Closeable
2. Created a new class called "LocalContext" or something that would allow us to have the effect of try-with-resources for what I think is still the common case?

That way people using the very old nested Context behavior don't need to change, and newer code that relies on try-with-resources would have to change to:

try (LocalContext lx = LocalContext.start()) { Context cx = lx.getContext(); // do stuff }

I feel like this is cleaner than relying on a "call" pattern, but doesn't break old Context behavior.

And once we do THAT, maybe someone could try to understand ContextFactory and the way it uses inheritance to define optional features, which might have been Java state-of-the-art in 1996 but I find very confusing today!