-
Notifications
You must be signed in to change notification settings - Fork 15
Feature ETP-3003: Enhance call sync #858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: prerelease/26.1
Are you sure you want to change the base?
Changes from 1 commit
494597b
4834e88
87e0d23
db61f24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| package com.etendoerp.db; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
|
|
||
| import org.openbravo.base.exception.OBException; | ||
| import org.openbravo.dal.core.OBContext; | ||
| import org.openbravo.dal.service.OBDal; | ||
| import org.openbravo.model.ad.process.ProcessInstance; | ||
| import org.openbravo.model.ad.ui.Process; | ||
| import org.openbravo.service.db.CallProcess; | ||
|
|
||
| /** | ||
| * Service class to execute database processes asynchronously. | ||
| * <p> | ||
| * This class extends {@link CallProcess} to inherit the core execution logic but runs the | ||
| * database procedure in a separate thread. This is useful for long-running processes | ||
| * to avoid blocking the user interface or triggering HTTP timeouts. | ||
| * </p> | ||
| * * <b>Key behaviors:</b> | ||
| * <ul> | ||
| * <li>Returns the {@link ProcessInstance} immediately with status 'Processing'.</li> | ||
| * <li>Manages the transfer of {@link OBContext} to the worker thread.</li> | ||
| * <li>Handles Hibernate session lifecycle (commit/close) for the background thread.</li> | ||
| * </ul> | ||
| */ | ||
| public class CallAsyncProcess extends CallProcess { | ||
|
|
||
| private static CallAsyncProcess instance = new CallAsyncProcess(); | ||
|
|
||
| // Thread pool to manage background executions. | ||
| // Using a fixed pool prevents system resource exhaustion. | ||
| private final ExecutorService executorService = Executors.newFixedThreadPool(10); | ||
|
|
||
| public static synchronized CallAsyncProcess getInstance() { | ||
| return instance; | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion Consider catching OBException separately or rethrowing it after updating the ProcessInstance, and use a proper logger instead of printing stack traces directly to stdout/stderr. + } catch (Exception e) {Rationale: Etendo guidelines favor using OBException to propagate business errors and using the logging framework (LogManager/Logger) instead of |
||
| public static synchronized void setInstance(CallAsyncProcess instance) { | ||
| CallAsyncProcess.instance = instance; | ||
| } | ||
|
|
||
| /** | ||
| * Overrides the main execution method to run asynchronously. | ||
| * * @param process | ||
| * the process definition. | ||
| * @param recordID | ||
| * the record ID. | ||
| * @param parameters | ||
| * map of parameters. | ||
| * @param doCommit | ||
| * explicit commit flag. | ||
| * @return the ProcessInstance in 'Processing' state (Result=0, Msg='Processing...'). | ||
| */ | ||
| @Override | ||
| public ProcessInstance callProcess(Process process, String recordID, Map<String, ?> parameters, Boolean doCommit) { | ||
| OBContext.setAdminMode(); | ||
| try { | ||
| // 1. SYNC PHASE: Prepare Data | ||
| // We must create the PInstance in the main thread to return the ID immediately to the user. | ||
| ProcessInstance pInstance = createAndPersistInstance(process, recordID, parameters); | ||
|
|
||
| // Set initial status specifically for Async (though createAndPersist usually sets defaults) | ||
| pInstance.setResult(0L); | ||
| pInstance.setErrorMsg("Processing in background..."); | ||
| OBDal.getInstance().save(pInstance); | ||
| OBDal.getInstance().flush(); | ||
|
|
||
| // Capture critical IDs and Context to pass to the thread | ||
| final String pInstanceId = pInstance.getId(); | ||
| final String processId = process.getId(); | ||
| final OBContext currentContext = OBContext.getOBContext(); | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [RESOLVED] > [!WARNING] Clone the current OBContext before passing it to another thread instead of reusing the same instance reference. + final OBContext currentContext = OBContext.getOBContext().clone();Rationale: OBContext is stored in a ThreadLocal and is not designed to be shared directly across threads. Passing the same instance reference to a different thread can lead to subtle concurrency issues if the context is mutated. Cloning the OBContext to hand off an independent copy is safer and better aligned with the documented ThreadLocal usage. |
||
| // 2. ASYNC PHASE: Submit to Executor | ||
| executorService.submit(() -> { | ||
| runInBackground(pInstanceId, processId, currentContext, doCommit); | ||
| }); | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [RESOLVED] > [!WARNING] Clone OBContext before handing it to another thread. Create a cloned context instance from OBContext.getOBContext() and pass that clone into the async runnable instead of the original ThreadLocal instance reference. + final OBContext currentContext = OBContext.getOBContext().clone();
+ executorService.submit(() -> runInBackground(pInstanceId, processId, currentContext, doCommit));Rationale: OBContext is implemented on top of ThreadLocal for per-thread isolation. Sharing the same OBContext instance across threads can cause subtle concurrency and security issues if the context state is mutated. Etendo recommendations for background/async processing emphasize creating an independent context per thread. |
||
| // 3. RETURN IMMEDIATELY | ||
| // The pInstance returned here is the initial snapshot. The UI should poll for updates. | ||
| return pInstance; | ||
|
|
||
| } finally { | ||
| OBContext.restorePreviousMode(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Internal method executed by the worker thread. | ||
| */ | ||
| private void runInBackground(String pInstanceId, String processId, OBContext context, Boolean doCommit) { | ||
| // A. Context Hydration | ||
| // The new thread does not have the user session. We must set it manually. | ||
| OBContext.setOBContext(context); | ||
| OBContext.setAdminMode(); | ||
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [RESOLVED] > [!WARNING] Remove the hydrateContext method and use the standard OBContext.setOBContext initialization. OBContext.setOBContext(contextValues.userId, contextValues.roleId, contextValues.clientId, contextValues.organizationId, contextValues.languageId, contextValues.warehouseId);Rationale: The hydrateContext method attempts to manually reconstruct the context state. This is error-prone and bypasses the framework's initialization logic. Using OBContext.setOBContext(IDs...) is the mandatory pattern for background threads. |
||
| try { | ||
| // B. Re-attach Hibernate Objects | ||
| // We cannot use the objects from the main thread (Process/ProcessInstance) | ||
| // because they belong to a different (likely closed) Hibernate Session. | ||
| ProcessInstance pInstance = OBDal.getInstance().get(ProcessInstance.class, pInstanceId); | ||
| Process process = OBDal.getInstance().get(Process.class, processId); | ||
|
|
||
| if (pInstance == null || process == null) { | ||
| throw new OBException("Async Execution Failed: Process Instance or Definition not found."); | ||
| } | ||
|
|
||
| // C. Execute Logic (Reusing CallProcess logic) | ||
| // This calls the protected method in the parent class (CallProcess) | ||
| executeStandardProcedure(pInstance, process, doCommit); | ||
|
|
||
| // D. Commit Transaction | ||
| // In async threads, we are responsible for the transaction boundary. | ||
|
||
| OBDal.getInstance().commitAndClose(); | ||
|
|
||
| } catch (Exception e) { | ||
| // E. Error Handling | ||
| // If something fails in the background, we must log it to the DB, | ||
| // otherwise the PInstance will remain "Processing..." forever. | ||
| try { | ||
| OBDal.getInstance().rollbackAndClose(); | ||
|
|
||
| // Open a new transaction to save the error | ||
| ProcessInstance pInstanceCtx = OBDal.getInstance().get(ProcessInstance.class, pInstanceId); | ||
| if (pInstanceCtx != null) { | ||
| pInstanceCtx.setResult(0L); // Error | ||
| String msg = e.getMessage() != null ? e.getMessage() : e.toString(); | ||
| // Truncate to avoid DB errors if message is too long | ||
| if (msg.length() > 2000) msg = msg.substring(0, 2000); | ||
| pInstanceCtx.setErrorMsg("Async Error: " + msg); | ||
|
||
|
|
||
| OBDal.getInstance().save(pInstanceCtx); | ||
| OBDal.getInstance().commitAndClose(); | ||
| } | ||
| } catch (Exception ex) { | ||
| // Catastrophic failure (DB down?), just log to console | ||
| ex.printStackTrace(); | ||
| } | ||
| e.printStackTrace(); | ||
| } finally { | ||
| OBContext.restorePreviousMode(); | ||
| } | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion Consider either implementing the intended asynchronous process invocation logic (following existing Etendo async/background process patterns) or removing this empty class until it is needed, to avoid leaving unused stubs in the codebase. +public class CallAsyncProcess {
+
+ // TODO: Implement async process invocation logic or remove this class if unused.
+Rationale: Empty public classes without fields or methods can cause confusion about their purpose, complicate maintenance, and risk becoming dead code. Etendo async/background processing usually follows well-defined patterns (e.g., DAL-based background processes or the asyncprocess module), so helper classes should encapsulate concrete behavior aligned with those patterns. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion
Extract the hard‑coded thread pool size into a named constant. This aligns with Etendo patterns for tunable limits and makes future adjustments or externalization easier.
Rationale: Magic numbers in concurrency configuration make it harder to reason about resource usage and to tune behavior in different environments. Etendo code typically uses named constants for limits and thresholds.