[Note: I'm still on abcl-1.4.0, which is what I refer to here.]

The original poster is right. And so, I always have to remember to write:

Interpreter.createInstance(); Interpreter interp = Interpreter.getInstance();

but OP's solution is faster.

Here is why there is a race. Interpreter.java looks like this:

public static synchronized Interpreter getInstance() { return interpreter; } public static synchronized Interpreter createInstance() { if (interpreter != null) return null; interpreter = new Interpreter(); ... return interpreter; }

OP's user code is:

Interpreter interpreter = Interpreter.getInstance (); if ( interpreter == null ) { interpreter = Interpreter.createInstance (); }

which is not itself surrounded by a synchronized (Interpreter.class) {...} block.

So two threads T1, T2 executing it could be scheduled in two possible ways, (where line execution is denoted by <thread, line_number>):

Schedule 1 <T1,1>, <T1,2>, <T2,1>, <T2,2>, <T2,3>, <T1,3> resulting in this T1:interpreter = null (due to Interpreter.createInstance()'s line 2): T2:interpreter = <not null>

Schedule 2 <T1,1>, <T1,2>, <T1,3>, <T2,1>, <T2,2>, <T2,3> resulting in: T1:interpreter = <not null> T2:interpreter = <not null>

On 11 Mar 2019, at 10:59, Mark Evenson evenson@panix.com wrote:

...

On Mar 11, 2019, at 06:38, dingd ntysdd@qq.com wrote:

this is recommended in the manual, it contains a race condition.

Interpreter interpreter = Interpreter.getInstance (); if ( interpreter == null ) { interpreter = Interpreter.createInstance (); }

getInstance and createInstance should be swapped.

Since both Interpreter.getInstance() and Interpreter.createInstance() are methods synchronized on the same object monitor, I don’t see that there is a race condition here. Could you explain a little more about your reasoning and/or experience with the race condition?

Two threads could call getInstance and have interpreter=null. Then both will call createInstance, thus two instances will be created. If interpreter is a local binding as it appears in the code above, then it is what’s intended and all is good. But if interpreter is a global binding and you expected interpreter to have a single instance, then it’s wrong.

I haven't looked at this code before, but in looking now, I am confused. Interpreter is intended to be a singleton, right? (that's what the comment says, as well as what is suggested by the use of the static interpreter variable.). In that case why would you want to createInstance to ever return null? Why isn't there just a single synchronized getSingleton() (call it getInterpreter if you want) method, which either returns returns the existing interpreter, or creates and initializes a new one and returns it.

Alan

On Mon, Mar 11, 2019 at 1:06 PM Mark Evenson evenson@panix.com wrote:

...

On Mar 11, 2019, at 16:18, Pascal Bourguignon pjb@informatimago.com

wrote:

...

...

On 11 Mar 2019, at 10:59, Mark Evenson evenson@panix.com wrote:

...

On Mar 11, 2019, at 06:38, dingd ntysdd@qq.com wrote:

this is recommended in the manual, it contains a race condition.

Interpreter interpreter = Interpreter.getInstance (); if ( interpreter == null ) { interpreter = Interpreter.createInstance (); }

getInstance and createInstance should be swapped.

Since both Interpreter.getInstance() and Interpreter.createInstance()

are

...

...

methods synchronized on the same object monitor, I don’t see that there

is a

...

...

race condition here. Could you explain a little more about your

reasoning

...

...

and/or experience with the race condition?

Two threads could call getInstance and have interpreter=null. Then both will call createInstance, thus two instances will be created. If interpreter is a local binding as it appears in the code above, then

it is what’s intended and all is good.

...

But if interpreter is a global binding and you expected interpreter to

have a single instance, then it’s wrong.

I am not sure what you mean by global/local bindings here, as these concepts don’t exist in Java as far as I know, but I think dingd is correct here: I don’t think two instances of the interpreter will be created.

As the Interpreter.createInstance() is synchronized on the Interpreter class (the “object monitor”), one of the calls to createInstance() will succeed in creating a value in the static Interpreter.interpreter reference, while the other will return null.

// Interface. public static synchronized Interpreter createInstance() { if (interpreter != null) return null; interpreter = new Interpreter(); _NOINFORM_.setSymbolValue(T); initializeLisp(); return interpreter; }

I’m still scratching my head over why the current implementation is so convoluted. Part of the problem is that we have three methods of creating an interpreter namely, createInstance(), createDefaultInstance() and createJLispInstance() with two different possible private constructors. If anyone has a good handle on the right architecture here, please pipe up.

The simplest path forward would be to change the documentation, as dingd originally suggested. But that path isn’t very satisfying.

-- "A screaming comes across the sky. It has happened before but there is nothing to compare to it now."

Ok, 3 getSingleton methods, one for each case, synchronized. First one wins. Subsequent ones throw an exception, since you can't have more than one.

On Tue, Mar 12, 2019 at 2:28 PM Mark Evenson evenson@panix.com wrote:

...

On Mar 11, 2019, at 21:47, Alan Ruttenberg alanruttenberg@gmail.com

wrote:

...

I haven't looked at this code before, but in looking now, I am confused. Interpreter is intended to be a singleton, right? (that's what the

comment says, as well as what is suggested by the use of the static interpreter variable.).

...

In that case why would you want to createInstance to ever return null? Why isn't there just a single synchronized getSingleton() (call it

getInterpreter if you want) method, which either returns returns the existing interpreter, or creates and initializes a new one and returns it.

My current take on the code is that createInstance() returns null due to there being three different code paths for initialization, so the “getInstance() calls create” can’t be wrapped in a single method.

The three different kinds of creation of the interpreter are:

1. createInstance() for “embedded" uses
2. createDefaultInstance() for bringing up the REPL
3. createJlispInstance() for use in the J editor

Each initialization path populates the static reference for the Interpreter singleton, and then proceeds to mutate the necessary state for its purposes. As such, the mere existence of the interpreter reference does not mean that all the necessary mutation has completed. In lieu of a proper semantic for having finished creation, each code path starts by checking to see if there is a value for interpreter, punting with a null value if this is not the case with the following stanza:

    if (interpreter != null)
        return null;

This is all very suboptimal from a user perspective, as getting a null from a call to create expecting the caller to handle the error is just wrong.

Still thinking about how to simplify this…

-- "A screaming comes across the sky. It has happened before but there is nothing to compare to it now."

Wasn't quite clear. I'm assuming that there are three constructors for a singleton and that they are incompatible - otherwise why not have one method? If I'm wrong about them being incompatible, then have only a single method and move the variance to another method.

I suggested exception because if one has already been created, and you ask for a now incompatible one, it should be an error. Calling the successful getSingleton again should succeed.

I'm assuming the getSingleton method(s) are synchronized on the same object (the Class in this case).

The advantage over the old way is that there isn't a race condition.

Actually, I'm not sure why only one interpreter is allowed. An alternative would be to have separate createInterpreter, getDefaultInterpreter or better, just use new to get a private interpreter instead of having a createInterpreter.

createInterpreter/new always succeeds and returns a non-global interpreter you need to keep track of yourself. getDefaultInterpreter either returns the static interpreter or creates, stores, and returns it.

Having a createXXX that returns nil and sometimes returns an interpreter makes no sense to me.

If we land up refactoring, I'd also propose we move the j stuff to a separate class.

On Wed, Mar 13, 2019 at 4:15 AM Mark Evenson evenson@panix.com wrote:

...

On Mar 12, 2019, at 21:10, Alan Ruttenberg alanruttenberg@gmail.com

wrote:

...

Ok, 3 getSingleton methods, one for each case, synchronized. First one wins. Subsequent ones throw an exception, since you can't have

more than one.

This doesn’t smell good to me.

One should be able to have multiple references to a singleton, as long as there is only one singleton. Consider the use of ABCL in a Java Servlet context where each instance of the servlet is processing a separate, parallel request. Each such Servlet begins its lifecycle by getting a reference to the singleton ABCL instance, and then calling into ABCL to service the request.

Maybe you meant “three create singleton methods, synchronized”? This is similar to what we have now, using an exception rather than returning a null on failure. This would be a breaking change for existing users of the interface, as they would have to change their code to deal with the exception (unless we throw something that inherits from RuntimeException which would be even worse). Philosophically, Java exceptions should not be used for anticipated failures, as they are not potentially restartable like Common Lisp conditions.

-- "A screaming comes across the sky. It has happened before but there is nothing to compare to it now."