-
-
Notifications
You must be signed in to change notification settings - Fork 483
[WFCORE-6934] Server startup marker file is not a reliable method to detect when the server is running (POC) #6756
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?
Changes from 1 commit
69bbe94
e1f5fc8
18e8429
1ffe8ea
1a341e3
e534e98
c773692
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 |
|---|---|---|
|
|
@@ -11,6 +11,11 @@ | |
| import java.io.IOException; | ||
| import java.net.InetAddress; | ||
| import java.net.InetSocketAddress; | ||
| import java.nio.channels.FileChannel; | ||
| import java.nio.channels.FileLock; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
| import java.nio.file.StandardOpenOption; | ||
| import java.security.PrivilegedAction; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
|
|
@@ -201,6 +206,9 @@ public JBossThreadFactory run() { | |
| configuration.setReadExecutor(Executors.newCachedThreadPool(threadFactory)); | ||
|
|
||
| final ProcessController processController = new ProcessController(configuration, System.out, System.err); | ||
|
|
||
| final FileChannel lockChannel = acquireRunningLock(jbossHome); | ||
|
|
||
| final InetSocketAddress boundAddress = processController.getServer().getBoundAddress(); | ||
|
|
||
| final List<String> initialCommand = new ArrayList<String>(); | ||
|
|
@@ -235,6 +243,7 @@ public JBossThreadFactory run() { | |
| final Thread shutdownThread = new Thread(new Runnable() { | ||
| public void run() { | ||
| processController.shutdown(); | ||
| releaseRunningLock(lockChannel); | ||
| } | ||
| }, "Shutdown thread"); | ||
| shutdownThread.setDaemon(false); | ||
|
|
@@ -243,6 +252,33 @@ public void run() { | |
| return processController; | ||
| } | ||
|
|
||
| private static FileChannel acquireRunningLock(String jbossHome) { | ||
| try { | ||
| Path lockPath = Paths.get(jbossHome, ".installation", "running.lock"); | ||
| java.nio.file.Files.createDirectories(lockPath.getParent()); | ||
| FileChannel channel = FileChannel.open(lockPath, | ||
| StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.READ); | ||
| FileLock lock = channel.tryLock(); | ||
| if (lock != null) { | ||
| return channel; | ||
| } | ||
|
Contributor
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. We have to decide what we want to do in this case. If the lock is not acquired, what does it mean? Do we want to start the server as usual? (I would say yes) If so, Prospero should not try to upgrade the server since we will never know whether the lock was legitimately acquired to signal that the server is running. We need to sort out this case and control it. If the server does not acquired the lock, by whatever reason, how Prospero would understand whether the server is still running? It seems to me we need a fallback for this case. That fallback case can be the old which uses temp directory, although moving it out of the temp directory. How to indicate to Prospero whether the server has legitimately acquired the log is still open. As first glance it could be a flag on the Installation Manager service that is written down into the properties file used to communicate to Prospero any context for the apply operation.
Contributor
Author
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. this just is for a test.... jvm has some sort of reentry for locks so it does not allow to acquire the lock twice (make sense), so if the lock is already taken... it should be by this jvm.
Contributor
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. ok, yes, we should not have other JVMs trying to get a lock ther .... just keep an eye on the embedded server / embedded Host Controller, I guess they won't start from main methods .. but just in case |
||
| channel.close(); | ||
| } catch (IOException e) { | ||
| // ignore | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static void releaseRunningLock(FileChannel channel) { | ||
| if (channel != null) { | ||
| try { | ||
| channel.close(); | ||
| } catch (IOException e) { | ||
| // ignore | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static boolean isJavaSecurityManagerConfigured(final String arg) { | ||
| return arg.startsWith("-Djava.security.manager") | ||
| && !"-Djava.security.manager=allow".equals(arg) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,21 @@ | |
| */ | ||
| package org.jboss.as.server; | ||
|
|
||
| import static java.security.AccessController.doPrivileged; | ||
|
|
||
| import java.io.BufferedWriter; | ||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.channels.FileChannel; | ||
| import java.nio.channels.FileLock; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.StandardOpenOption; | ||
| import java.security.PrivilegedActionException; | ||
| import java.security.PrivilegedExceptionAction; | ||
|
|
||
| import org.wildfly.security.manager.WildFlySecurityManager; | ||
|
|
||
| import org.jboss.as.network.NetworkUtils; | ||
| import org.jboss.as.server.logging.ServerLogger; | ||
|
|
@@ -28,6 +37,8 @@ | |
| public final class BootstrapListener { | ||
|
|
||
| public static final String MARKER_FILE = "startup-marker"; | ||
| public static final String RUNNING_LOCK_FILE = "running.lock"; | ||
| private static final String INSTALLATION_DIR = ".installation"; | ||
|
|
||
| private final StabilityMonitor monitor = new StabilityMonitor(); | ||
| private final ServiceContainer serviceContainer; | ||
|
|
@@ -36,8 +47,10 @@ public final class BootstrapListener { | |
| private final String prettyVersion; | ||
| private final FutureServiceContainer futureContainer; | ||
| private final File tempDir; | ||
| private String startedCleanMessage; | ||
| private String startedWitErrorsMessage; | ||
| private String startedCleanMessage; | ||
| private String startedWitErrorsMessage; | ||
| private volatile FileChannel lockFileChannel; | ||
|
Contributor
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. There is still an unnecessary indirection between the BootstrapListener and the ApplicationServerService for this. From my point if view, changes could be simplified if all the logic to acquire / release the lock is in the ApplicationServerService. Right now the ApplicationServerService uses the BootstrapListener for acquire/releasing the lock on each server start/reload and looks unnecessary. This is still a risk since during the reload, the advisory lock will be released, meanwhile the server is still started. |
||
| private volatile FileLock runningLock; | ||
|
|
||
| public BootstrapListener(final ServiceContainer serviceContainer, final ElapsedTime elapsedTime, final ServiceTarget serviceTarget, final FutureServiceContainer futureContainer, final String prettyVersion, final File tempDir) { | ||
| this.serviceContainer = serviceContainer; | ||
|
|
@@ -130,7 +143,6 @@ private void createStartupMarker(String result, long startTime) { | |
| } catch (IOException e) { | ||
| // ignore | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public static void deleteStartupMarker(File tempDir) { | ||
|
|
@@ -142,6 +154,47 @@ public static void deleteStartupMarker(File tempDir) { | |
| } | ||
| } | ||
|
|
||
| public void acquireRunningLock(File homeDir) { | ||
| final Path lockPath = homeDir.toPath().resolve(INSTALLATION_DIR).resolve(RUNNING_LOCK_FILE); | ||
| try { | ||
| if (WildFlySecurityManager.isChecking()) { | ||
| doPrivileged((PrivilegedExceptionAction<Void>) () -> { | ||
| openLockFile(lockPath); | ||
| return null; | ||
| }); | ||
| } else { | ||
| openLockFile(lockPath); | ||
| } | ||
| } catch (PrivilegedActionException | IOException e) { | ||
|
Contributor
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. We no longer use security manager, so we do not need to check for permissions. |
||
| // ignore | ||
| } | ||
| } | ||
|
|
||
| private void openLockFile(Path lockPath) throws IOException { | ||
| Files.createDirectories(lockPath.getParent()); | ||
| FileChannel channel = FileChannel.open(lockPath, | ||
| StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.READ); | ||
| FileLock lock = channel.tryLock(); | ||
| if (lock != null) { | ||
| lockFileChannel = channel; | ||
| runningLock = lock; | ||
| } else { | ||
| channel.close(); | ||
| } | ||
| } | ||
|
|
||
| public void releaseRunningLock() { | ||
| if (lockFileChannel != null) { | ||
| try { | ||
| lockFileChannel.close(); | ||
| } catch (IOException e) { | ||
| // ignore | ||
|
Contributor
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. Do not silently ignore it |
||
| } | ||
| lockFileChannel = null; | ||
| runningLock = null; | ||
| } | ||
| } | ||
|
|
||
| public void logAdminConsole() { | ||
| ServiceController<?> controller = serviceContainer.getService(UndertowHttpManagementService.SERVICE_NAME); | ||
| if (controller != null) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.