PAYARA-2889 Defer OpenAPI Scanning Until Endpoint Visit

[MattGill98]

• Deferred OpenAPI scanning until the /openapi endpoint is visited.
• Allowed document creation for apps deployed while the OpenAPI service is disabled, since it's now created on endpoint visiting.
• Added a null check to prevent potential @Produces errors.

[MattGill98]

jenkins test please

[payara-ci]

Quick build and test passed!

[OndroMih]

This looks like multiple tasks for the same document can be submitted, right? Also, if the executor uses multiple threads in a pool, the tasks can be executed in parallel.

If I'm right, then we have to ensure that the document is submitted to the executor only once.

Also, just in case, OpenApiBuilder.run() should only do its job only if the document is not yet done / the completableFuture is not completed.

[MattGill98]

The method is synchronized for this reason, it can only be run in one thread at a time. Since the wait loop is in there, all threads will wait for the document to be built.

[MattGill98]

I've just added the second check you asked for anyway, since there's no harm in adding it!

[OndroMih]

I missed that the code is in the !document.isDone block so it's only executed when document isn't done yet.

However, this is not thread safe because it's submitted to the executor, which runs the builder in another thread. While a document is being generated, getDocument can be executed in another thread, which will schedule another builder.

Safe method to run this only once is to add a handler to the CompletableFuture document immediately when it's created and then only complete the future in the executor. The handler would be executed only the first time the future is completed.

Something like in the constructor:

 this.document = new CompletableFuture<>();
this.document.thenRun(new OpenApiBuilder(this)); // no need to pass this.document as argument as the builder doesn't need to interact with it

and in getDocument() simply:

if (! this.document.isDone() ) {  // this condition is only to optimize so that the task isn't submitted to the executor at all because it would be ignored anyway
  executor.submit( () -> this.document.complete(null) );  // this triggers the above handler only the first time when the document is completed, otherwise it's ignored
}

[MattGill98]

I believe it's already thread safe. It submits the task to the executor, and then waits in the same method for it to be finished.

[OndroMih]

It's not entirely thread-safe, see my comment below on line 283.

[MattGill98]

jenkins test please

[payara-ci]

Quick build and test passed!

[OndroMih]

This check is not thread safe - if the executor has more threads, the run() method can be executed concurrently. Before the first one is finished, the future isn't done so the second one will also try to do the job and they will both run in parallel.

The call to future.isDone() isn't atomic and the future might be being completed in a parallel thread while not yet done.

[OndroMih]

It's not entirely thread-safe, see my comment below on line 283.

[OndroMih]

Hi @MattGill98 the current solution is thread-safe and I'll approve.

[MattGill98]

jenkins test please

[payara-ci]

Quick build and test passed!