Skip to content
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

Update ThreadUtils.java #1376

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

XMRZombie
Copy link
Contributor

Validation Added: Introduced validateThreadId method to ensure that the threadId is neither null nor empty before processing.

Executor Retrieval Simplified: Used computeIfAbsent for cleaner and more efficient executor retrieval in the execute method.

Thread Name Management: Improved the naming of threads within the executor context for better traceability.

Thread Removal Logic: Enhanced the logic for removing thread mappings to ensure thread safety and correctness.

Restored Interrupt Status: Ensured that the interrupted status of the thread is restored after handling an InterruptedException.

Cleaner Shutdown Logic: Improved the shutdown process to remove the executor from the map while also returning it, ensuring that the pool is only shutdown if it exists.

Validation Added: Introduced validateThreadId method to ensure that the threadId is neither null nor empty before processing.

Executor Retrieval Simplified: Used computeIfAbsent for cleaner and more efficient executor retrieval in the execute method.

Thread Name Management: Improved the naming of threads within the executor context for better traceability.

Thread Removal Logic: Enhanced the logic for removing thread mappings to ensure thread safety and correctness.

Restored Interrupt Status: Ensured that the interrupted status of the thread is restored after handling an InterruptedException.

Cleaner Shutdown Logic: Improved the shutdown process to remove the executor from the map while also returning it, ensuring that the pool is only shutdown if it exists.
@woodser
Copy link
Contributor

woodser commented Nov 4, 2024

The application is highly sensitive to changes in this file.

Your pull request is making functional changes, such as removing the thread mapping when a task is done executing, and I can already see that at least one bug is introduced (THREADS.put is never called).

Was there some functional problem which motivated these changes, or just the desire for general improvements?

Given the sensitivity of this code, I suggest first starting with non or minimally functional improvements for a PR (e.g. add validateThreadId, computeIfAbsent, and restore interrupt status).

As of now, I see no functional reason to merge the other changes but plenty of risk and need for retesting.

Preserved THREAD calls

Summary of Key Changes:

- Thread ID Validation:
 The validateThreadId method has been added to ensure that a thread ID cannot be null or empty. This is called at the start of relevant methods like execute and shutDown.

 - Shut Down Logic:
The shutDown method uses the validateThreadId method to ensure the threadId is valid before proceeding with shutdown logic.

- Task Execution and Awaiting:
The execute, await, and awaitTasks methods ensure that tasks are properly submitted and awaited, with timeout handling if provided.

- Thread Safety:
Thread safety is maintained in all methods where shared resources (EXECUTORS and THREADS) are accessed, using synchronized blocks to ensure proper handling in multi-threaded environments.

- Explanation of Major Components:
- EXECUTORS and THREADS Maps: These maps are used to store the ExecutorService and the current Thread associated with a specific threadId. They allow tracking which thread is running which executor.

- POOL: A default pool for running tasks that don't require a specific thread.

- awaitTasks: This method executes multiple tasks concurrently with a configurable level of concurrency, and waits for their completion, optionally handling timeouts.

- Thread Naming: Threads are named with their threadId for better traceability in logs.
@XMRZombie
Copy link
Contributor Author

Yes it was a desire to contribute and improve how threading is handled on haveno

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants