-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix for postgres instance process closing #12927
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: main
Are you sure you want to change the base?
Conversation
…ination (Fix JabRef#12844) * Added logic to detect and clean up stale embedded PostgreSQL instances left behind by previous JabRef sessions. * Introduced PostgresProcessCleaner to safely shut down any orphaned PostgreSQL processes using PID-based detection. * Registered a shutdown hook in Launcher to ensure embedded PostgreSQL is properly terminated during normal or abrupt shutdown.
…ination (Fix JabRef#12844) * Added logic to detect and clean up stale embedded PostgreSQL instances left behind by previous JabRef sessions. * Introduced PostgresProcessCleaner to safely shut down any orphaned PostgreSQL processes using PID-based detection. * Registered a shutdown hook in Launcher to ensure embedded PostgreSQL is properly terminated during normal or abrupt shutdown. * Updated CHANGELOG.md
…ination (Fix JabRef#12844) * Added logic to detect and clean up stale embedded PostgreSQL instances left behind by previous JabRef sessions. * Introduced PostgresProcessCleaner to safely shut down any orphaned PostgreSQL processes using PID-based detection. * Registered a shutdown hook in Launcher to ensure embedded PostgreSQL is properly terminated during normal or abrupt shutdown.
…ination (Fix JabRef#12844) * Added logic to detect and clean up stale embedded PostgreSQL instances left behind by previous JabRef sessions. * Introduced PostgresProcessCleaner to safely shut down any orphaned PostgreSQL processes using PID-based detection. * Registered a shutdown hook in Launcher to ensure embedded PostgreSQL is properly terminated during normal or abrupt shutdown. * Updated CHANGELOG.md
…ination (Fix JabRef#12844) * Added logic to detect and clean up stale embedded PostgreSQL instances left behind by previous JabRef sessions. * Introduced PostgresProcessCleaner to safely shut down any orphaned PostgreSQL processes using PID-based detection. * Registered a shutdown hook in Launcher to ensure embedded PostgreSQL is properly terminated during normal or abrupt shutdown. * Updated CHANGELOG.md
…ination (Fix JabRef#12844) * style changes in PostgreProcessCleaner, Launcher
|
||
private void destroyPreviousJavaProcess(Map<String, Object> meta) throws InterruptedException { | ||
long javaPid = ((Number) meta.get("javaPid")).longValue(); | ||
destroyProcessByPID(javaPid, 1000); |
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.
How does one know whether this stores JabRef data? Shouldn't a connection attempt be made?
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.
In writeMetadataToFile method, I am using ProcessHandle.current().pid(), so that the current running process id will be stored in the json file.
This is AI generated, isn't it? |
Have you tested this by observing the processes? If so, on which OS? |
nope. |
import static org.jabref.model.search.PostgreConstants.BIB_FIELDS_SCHEME; | ||
|
||
public class PostgreServer { | ||
private static final Logger LOGGER = LoggerFactory.getLogger(PostgreServer.class); | ||
public static final Path POSTGRES_METADATA_FILE = Path.of("/tmp/jabref-postgres-info.json"); |
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.
Will not work in Windows. Will not work if multiple JabRef instances run.
For the former, AppDirs can be used (can't it?) - for the latter, the whole flow needs to be thought through.
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.
Got it, I need to check about AppDirs once & get back on this.
yes, I have tested on macOS Sequoia 15.4. I will check on windows and update here. |
Interesting - this seems to be going in a positive direction, if we can fix it for Windows. |
…ination (Fix JabRef#12844) * style changes in PostgreServer imports
…ination (Fix JabRef#12844) * Updated CHANGELOG.md
…ination (Fix JabRef#12844) * Modified PostgreProcessCleaner logic to fix windows os issue.
checked on windows as well, can you please check from your end as well? screen-capture-windows.mp4 |
…ination (Fix JabRef#12844) * Used Path.of() instead of Paths.get() in PostgreProcessCleaner.
Embedded Postgres Cleanup Flow 2.Write Metadata File 3.Check for Metadata Files on Startup 4.Verify Java Process Liveness 5.Handle Stale Java Processes 6.Lookup Postgres Processes 7.Kill Stale Postgres 8.Delete Metadata Files 9.Proceed with Fresh Initialization 10. Multiple Instance Safety |
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
…ination (Fix JabRef#12844) * Fix for trag-bot comments.
* Fix for trag-bot comments.
* Fix for trag-bot comments.
* Fix for trag-bot comments. * Created separate class PostgresMetadataWriter to follow SRP.
* Fix for trag-bot comments.
Map<String, Object> meta = new HashMap<>(); | ||
meta.put("javaPid", ProcessHandle.current().pid()); | ||
meta.put("postgresPort", port); | ||
meta.put("startedBy", "jabref"); |
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.
The string 'jabref' is a magic string and should be defined as a constant to improve maintainability and readability.
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.
In some sense, trag-bot is right.
Is it possible to, maybe, make a serializable class?
@trag-bot didn't find any issues in the code! ✅✨ |
This comment was marked as resolved.
This comment was marked as resolved.
Map<String, Object> metadata = new HashMap<>(OBJECT_MAPPER.readValue(Files.readAllBytes(metadataPath), HashMap.class)); | ||
long javaPid = ((Number) metadata.get("javaPid")).longValue(); | ||
if (isJavaProcessAlive(javaPid)) { | ||
return; |
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.
If the database is still running, is it necessary to clean up the data to ensure it's in the same state as a freshly started database?
} | ||
int postgresPort = ((Number) metadata.get("postgresPort")).intValue(); | ||
destroyPostgresProcess(postgresPort); | ||
Files.deleteIfExists(metadataPath); |
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.
Interesting question—what happens if you're running multiple JabRef instances? How can you distinguish between them?
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.
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.
Yeah, the question was for the other case.
Maybe, we should just start to disallow multiple instances?
.redirectErrorStream(true).start(); | ||
} else if (OS.WINDOWS) { | ||
return executeWindowsCommand(port); | ||
} |
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.
Minor question: Is lsof included by default on macOS and Linux? For example, if someone is running Ubuntu with a standard configuration—without adding any extra packages—the solution should ideally work out of the box.
Additionally, there could be security or access rights issues. For instance, on Windows, a domain admin might block users from executing cmd.exe.
At the very least, the documentation should specify the conditions under which the solution is expected to run successfully.
} | ||
|
||
public static Path getMetadataFilePath() { | ||
return Path.of(System.getProperty("java.io.tmpdir"), |
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.
I think the org.jabref.logic.util.Directories class should be used here to obtain the directory in a standardized way.
private Process executeWindowsCommand(int port) throws IOException { | ||
String systemRoot = System.getenv("SystemRoot"); | ||
if (systemRoot != null && !systemRoot.isBlank()) { | ||
String netStatPath = systemRoot + "\\System32\\netstat.exe"; |
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.
Is it possible to use Java paths?
if (systemRoot != null && !systemRoot.isBlank()) { | ||
String netStatPath = systemRoot + "\\System32\\netstat.exe"; | ||
String findStrPath = systemRoot + "\\System32\\findstr.exe"; | ||
String command = netStatPath + " -ano | " + findStrPath + " :" + port; |
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.
Hmm.. is there an API, where you specify this for running a process:
- Command name
- List of strings that represent arguments
I think this is more stable than just passing a string
if (systemRoot != null && !systemRoot.isBlank()) { | ||
String netStatPath = systemRoot + "\\System32\\netstat.exe"; | ||
String findStrPath = systemRoot + "\\System32\\findstr.exe"; | ||
String command = netStatPath + " -ano | " + findStrPath + " :" + port; |
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.
What does |
do here? It's like a pipe from Unix? Does it work on Windows, or it has other meaning?
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.
findstr should be programmed in Java
Code reads like AI generated...
tasklist ist the proposal by "the internet" - https://www.rgagnon.com/javadetails/java-0593.html
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.
With java 9 + we can use Process Handles https://stackoverflow.com/a/45068036
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.
findstr should be programmed in Java
Code reads like AI generated...
tasklist ist the proposal by "the internet" - https://www.rgagnon.com/javadetails/java-0593.html
Not AI, I have referred these for executing commands:
https://coderanch.com/t/688902/java/Executing-netstat-command-process-builder
https://mkyong.com/java/java-processbuilder-examples/
Also I will check the commands we need to program using java, We can make use of command design pattern, so that if any other command needs to be implemented, we can easily extend that as well
return null; | ||
} | ||
|
||
private long extractPidFromOutput(BufferedReader reader) throws IOException { |
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.
So you call some external command to get PID, right?
I wonder, is there a nicer package for Java? Or some alternative in packages that we have.. (If there is no such thing, then I think it's okay to use commands)
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.
I tried google, did not find something immediatly.
Fixes #12844
Summary
PostgreProcessCleaner
to safely shut down any orphaned PostgreSQL processes using port-based detection and Java PID tracking.Launcher
to ensure embedded PostgreSQL is properly terminated during normal or abrupt shutdowns (excluding SIGKILL).Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)macOS Sequoia 15.4.
https://www.loom.com/share/9ed0da7815714797b4f0568c33f6df03?sid=34114b6c-ca32-43ff-9896-bfae39b36c81
Windows
https://github.com/user-attachments/assets/aa7a7da4-661e-464e-a08f-f6befba66363