I'm not sure whether "Java_org_jpl7_fli_Prolog_attach_1pool_1engine" actually unlocks the engine array in all cases

[dtonhofer]

I have let the magic of Eclipse C IDE work on jpl.c.

It complained that Java_org_jpl7_fli_Prolog_attach_1pool_1engine does not always properly return a value. It is right.

Then I noticed that with some likelihood, the engines array is not properly unlocked under circumstances of error.

The code was hard to read so I reengineered it as below (using a state variable instead of a goto just feels better).

It compiles but I haven't tried to run it yet (so there is some likelihood of non-success). Well, tell me what you think or if it's of any use.

(It seems that the C code tries to stay conservatively in before-C99 style, feels very "automotive industry"?. Also, the restriction on 80 columns is very old-school.)

Enough grumbling:

/*
 * Some signaling constants used soon. They implement a state machine (sort of)
 */

static const int SUCCESS_AND_UNLOCKED = 1;
static const int FAILURE_AND_UNLOCKED = 2;
static const int TRY_NEXT_STILL_LOCKED = 3;

/*
 * Class:     org_jpl7_fli_Prolog
 * Method:    attach_pool_engine
 * Signature: ()Lorg/jpl7/fli/engine_t;
 */
JNIEXPORT jobject JNICALL
Java_org_jpl7_fli_Prolog_attach_1pool_1engine(JNIEnv *env, jclass jProlog) {

        if (!jpl_ensure_pvm_init(env)) {
                return NULL; // FIXME: throw exception (from Java)
        }

        // Find a Prolog engine to use. Try setting each of the engines in the pool.
        // If they are all in use, wait for the condition variable and then try again.
        // FIXME: Shouldn't there be at least a timeout?

        LOCK_ENGINES();

        jobject rval;
        int res = TRY_NEXT_STILL_LOCKED;

        while (res == TRY_NEXT_STILL_LOCKED) {
                 int empty_pos = -1;
                 for (int i = 0; ((i < engines_allocated) && (res == TRY_NEXT_STILL_LOCKED)); i++) {
                        if (engines[i] != NULL) {
                                // Found a live engine. Attaching to it may or may not succeed
                                // On success, the lock on engine[i] has been released already.
                                res = try_attaching_to_pool_engine(i, env, &rval);
                        }
                        else {
                                if (empty_pos == -1) {
                                        empty_pos = i; // retain earliest free entry for reallocation
                                }
                        }
                 }
                 if (res == TRY_NEXT_STILL_LOCKED) {
                         // We ran out of engines; try allocating a new one on a free place
                         if (empty_pos >= 0) {
                                 // There still is a free place; the engine becomes non-null on success
                                 create_pool_engine(empty_pos);
                                 if (engines[empty_pos] == NULL) {
                                         // failed to created engine; busted for good!
                                         UNLOCK_ENGINES();
                                         res = FAILURE_AND_UNLOCKED;
                                 }
                         }
                         else {
                                 unlock_engines_and_wait_patiently();
                         }
                }
        }

        if (res == SUCCESS_AND_UNLOCKED) {
                return rval;
        }
        else {
                assert(res == FAILURE_AND_UNLOCKED);
                return NULL; // TODO Java should raise an exception
        }
}

/* unlock engines and wait patiently */

static void
unlock_engines_and_wait_patiently() {
         DEBUG(1, Sdprintf("JPL no engines ready; waiting...\n"));
#ifdef USE_WIN_EVENTS
         UNLOCK_ENGINES();
         WaitForSingleObject(engines_event, 1000);
         LOCK_ENGINES();
#else
         pthread_cond_wait(&engines_cond, &engines_mutex);
#endif
}

/* create an engine at position i (we have the lock on engines) */

static void
create_pool_engine(int i)
{
        // https://eu.swi-prolog.org/pldoc/doc_for?object=c(%27PL_create_engine%27)
        engines[i] = PL_create_engine(NULL);
        if (engines[i] == NULL) {
                Sdprintf("JPL: Failed to create engine %d\n", i);
        }
        else {
                DEBUG(1, Sdprintf("JPL created engine[%d]=%p\n", i, engines[i]));
        }
}

/*
 * Call PL_set_engine on engine i to own it; if this succeeds, immediately
 * release the lock on engines[]. Returns NULL or pointer to the engine (jEngineT_c)
 */

static int
try_attaching_to_pool_engine(int i, JNIEnv *env, jobject *rval)
{
        DEBUG(1, Sdprintf("JPL trying engine[%d]=%p\n", i, engines[i]));
        // https://eu.swi-prolog.org/pldoc/doc_for?object=c(%27PL_set_engine%27)
        int rc = PL_set_engine(engines[i], NULL);
        if (rc == PL_ENGINE_SET) {
                DEBUG(1,Sdprintf("JPL attaching engine[%d]=%p\n", i, engines[i]));
                UNLOCK_ENGINES();
                // https://docs.oracle.com/en/java/javase/14/docs/specs/jni/functions.html#allocobject
                *rval = (*env)->AllocObject(env, jEngineT_c);
                if (*rval != NULL) {
                        setPointerValue(env, *rval, (pointer) engines[i]);
                        return SUCCESS_AND_UNLOCKED;
                }
                else {
                        // I don't know what this does:
                        PL_set_engine(NULL, NULL);
                        return FAILURE_AND_UNLOCKED;
                }
        }
        else if (rc == PL_ENGINE_INUSE) {
                return TRY_NEXT_STILL_LOCKED;
        }
        else {
                DEBUG(1,Sdprintf("JPL PL_set_engine %d fails with %d\n", i,     rc));
                UNLOCK_ENGINES();
                return FAILURE_AND_UNLOCKED;
        }
}

vs the original which is compressed as for embedded MCUs, but hard to ascertain:

/*
 * Class:     org_jpl7_fli_Prolog
 * Method:    attach_pool_engine
 * Signature: ()Lorg/jpl7/fli/engine_t;
 */
JNIEXPORT jobject JNICALL
Java_org_jpl7_fli_Prolog_attach_1pool_1engine(JNIEnv *env, jclass jProlog)
{ jobject rval;
  int     i;

  if ( !jpl_ensure_pvm_init(env) )
    return NULL;                                /* FIXME: throw exception */

  /* Find an engine. Try setting each of the engines in the pool. */
  /* If they are all in use wait for the condition variable and try again. */
  LOCK_ENGINES();
  for (;;)
  { try_again:
    for (i = 0; i < engines_allocated; i++)
    { int rc;

      if ( !engines[i] )
        continue;

      DEBUG(1, Sdprintf("JPL trying engine[%d]=%p\n", i, engines[i]));
      if ((rc = PL_set_engine(engines[i], NULL)) == PL_ENGINE_SET)
      { DEBUG(1, Sdprintf("JPL attaching engine[%d]=%p\n", i, engines[i]));
        UNLOCK_ENGINES();

        if ( (rval = (*env)->AllocObject(env, jEngineT_c)) )
        { setPointerValue(env, rval, (pointer)engines[i]);
          return rval;
        }

        PL_set_engine(NULL, NULL);
        return NULL;
      }
      if ( rc != PL_ENGINE_INUSE )
      { DEBUG(1, Sdprintf("JPL PL_set_engine %d fails with %d\n", i, rc));
        UNLOCK_ENGINES();
        return NULL; /* bad engine status: oughta throw exception */
      }
    }

    for (i = 0; i < engines_allocated; i++)
    { if ( !engines[i] )
      { if ( !(engines[i] = PL_create_engine(NULL)) )
        { Sdprintf("JPL: Failed to create engine %d\n", i);
          return NULL;
        }

        DEBUG(1, Sdprintf("JPL created engine[%d]=%p\n", i, engines[i]));

        goto try_again;
      }
    }

    DEBUG(1, Sdprintf("JPL no engines ready; waiting...\n"));
#ifdef USE_WIN_EVENTS
    UNLOCK_ENGINES();
    WaitForSingleObject(engines_event, 1000);
    LOCK_ENGINES();
#else
    pthread_cond_wait(&engines_cond, &engines_mutex);
#endif
  }
}

[JanWielemaker]

Thanks for spotting the lacking unlock. Added. The rest is a bit harder. Some remarks on how I see the status of jpl.c.

• First of all, the layout is now consistent with the rest of SWI-Prolog. I copied this layout from my C guru in the days I was learning the language (Anjo Anjewierden). I don't think it is bad in itself, but it is unusual. We had a rather long discussion on whether or not to switch to something more common these days a couple of years back with those involved in the C code. The end result was that there was no consensus where to switch too, automatic reformatting would probably garble many now carefully aligned and formatted code (tested with several) tools and breaking the history (git blame) is a high price. It is possible to find relevant git commits over a layout change, bit it is not comfortable. So, we decided to stick with what is there.

• IMO, the biggest flaw of jpl.c is the use of far too many macros. Functions are way easier to debug. Related is a high number of casts rather than relying on implicit type conversions and have the compiler warn that bits are lost. This caused Issues using JPL on Windows and JDK > 8 #34 so long to fix.

• A next good step would be to throw exceptions from the JNI functions when appropriate. The need for that is in the comments in many places, but it happens rarely (or not at all?) This makes JPL (I guess) rather shaky when unexpected things happen. SWI-Prolog itself isn't too good at dealing with an overall shortage of memory, but it deal quite well with many (stack) resource exceptions and many other things that can go wrong in the API.

As for state variables, I do not know. I don't really consider them better in general. I agree that dealing with many return paths in a function that uses locking should not be done as it is here. I typically use one of two options:

• Have a failure block at the end of the function that does all the required cleanup (free resources, locks, etc.) and jump there instead of using return.
• Put the complex thing with many return paths in a function and simply lock()/unlock() around the function call. This is probably appropriate here.

In general I'm not in favor of large rewrites unless needed. Yes, there are moments that the mess got big enough to rethink the whole thing again. If that turns the mess into something that is so clear that it is obviously correct, great. If it merely turns it into something that it still far from trivial to grasp and especially when there is no extensive regression test suite, I rather fix what is there and rely on git to tell me what happened to the code when and why. This is particularly the case for code that lacks a person with a complete overview that is in charge.

For short, we either come with a good plan how to further develop JPL and who takes responsibility of JPL for the next (say) 3 years or we stick with incremental fixes and improvements. I'm happy with either, but I'm not that person.