You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/05/02 15:25:28 UTC
[GitHub] [activemq-artemis] jbertram opened a new pull request, #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
jbertram opened a new pull request, #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061
It would be useful to be able to cycle the embedded web server if, for
example, one needed to renew the SSL certificates. Supporting this
functionality required a number of changes such as:
- Adding `getName()` to `ActiveMQComponent` along with a default
implementation. This was necessary in order to actually find the
`WebServerComponent` in the broker's list of components. Many classes
which implement `ActiveMQComponent` already had `getName()` but a
handful of them were returning `String` rather than `SimpleString` so
they had to be updated.
- Refactoring `WebServerComponent` so that all the necessary
configuration would happen in the `start()` method.
- Refactoring `WebServerComponentTest` to re-use code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871717708
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -193,87 +203,97 @@ public void configure(ComponentDTO config, String artemisInstance, String artemi
instanceContext.setResourceBase(instanceWarDir.toString());
instanceContext.setHandler(instanceResourceHandler);
instanceContext.setVirtualHosts(virtualHosts);
- homeContext.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false");
+ homeContext.setInitParameter(DIR_ALLOWED, "false");
DefaultHandler defaultHandler = new DefaultHandler();
defaultHandler.setServeIcon(false);
- if (webServerConfig.requestLog != null) {
+ if (this.config.requestLog != null) {
handlers.addHandler(getLogHandler());
}
handlers.addHandler(homeContext);
handlers.addHandler(instanceContext);
handlers.addHandler(defaultHandler); // this should be last
server.setHandler(handlers);
- }
- private RequestLogHandler getLogHandler() {
- RequestLogWriter requestLogWriter = new RequestLogWriter();
- CustomRequestLog requestLog;
+ cleanupTmp();
+ server.start();
- // required via config so no check necessary
- requestLogWriter.setFilename(webServerConfig.requestLog.filename);
+ ActiveMQWebLogger.LOGGER.webserverStarted(bindings
+ .stream()
+ .map(binding -> binding.uri)
+ .collect(Collectors.joining(", ")));
- if (webServerConfig.requestLog.append != null) {
- requestLogWriter.setAppend(webServerConfig.requestLog.append);
- }
+ ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
+ ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ }
- if (webServerConfig.requestLog.filenameDateFormat != null) {
- requestLogWriter.setFilenameDateFormat(webServerConfig.requestLog.filenameDateFormat);
- }
+ private ServerConnector createServerConnector(HttpConfiguration httpConfiguration,
+ int i,
+ BindingDTO binding,
+ URI uri,
+ String scheme) throws Exception {
+ ServerConnector connector;
- if (webServerConfig.requestLog.retainDays != null) {
- requestLogWriter.setRetainDays(webServerConfig.requestLog.retainDays);
- }
+ if ("https".equals(scheme)) {
+ SslContextFactory.Server sslFactory = new SslContextFactory.Server();
+ sslFactory.setKeyStorePath(binding.keyStorePath == null ? artemisInstance + "/etc/keystore.jks" : binding.keyStorePath);
+ sslFactory.setKeyStorePassword(binding.getKeyStorePassword() == null ? "password" : binding.getKeyStorePassword());
- if (webServerConfig.requestLog.format != null) {
- requestLog = new CustomRequestLog(requestLogWriter, webServerConfig.requestLog.format);
- } else if (webServerConfig.requestLog.extended != null && webServerConfig.requestLog.extended) {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT);
- } else {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.NCSA_FORMAT);
- }
-
- if (webServerConfig.requestLog.ignorePaths != null && webServerConfig.requestLog.ignorePaths.length() > 0) {
- String[] split = webServerConfig.requestLog.ignorePaths.split(",");
- String[] ignorePaths = new String[split.length];
- for (int i = 0; i < ignorePaths.length; i++) {
- ignorePaths[i] = split[i].trim();
+ if (binding.getIncludedTLSProtocols() != null) {
+ sslFactory.setIncludeProtocols(binding.getIncludedTLSProtocols());
+ }
+ if (binding.getExcludedTLSProtocols() != null) {
+ sslFactory.setExcludeProtocols(binding.getExcludedTLSProtocols());
+ }
+ if (binding.getIncludedCipherSuites() != null) {
+ sslFactory.setIncludeCipherSuites(binding.getIncludedCipherSuites());
+ }
+ if (binding.getExcludedCipherSuites() != null) {
+ sslFactory.setExcludeCipherSuites(binding.getExcludedCipherSuites());
+ }
+ if (binding.clientAuth != null) {
+ sslFactory.setNeedClientAuth(binding.clientAuth);
+ if (binding.clientAuth) {
+ sslFactory.setTrustStorePath(binding.trustStorePath);
+ sslFactory.setTrustStorePassword(binding.getTrustStorePassword());
+ }
}
- requestLog.setIgnorePaths(ignorePaths);
- }
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- return requestLogHandler;
- }
+ SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- @Override
- public void start() throws Exception {
- if (isStarted()) {
- return;
- }
- cleanupTmp();
- server.start();
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ httpConfiguration.setSendServerVersion(false);
+ HttpConnectionFactory httpFactory = new HttpConnectionFactory(httpConfiguration);
- String bindings = webServerConfig.getBindings()
- .stream()
- .map(binding -> binding.uri)
- .collect(Collectors.joining(", "));
- ActiveMQWebLogger.LOGGER.webserverStarted(bindings);
+ connector = new ServerConnector(server, sslConnectionFactory, httpFactory);
- ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
- ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ } else {
+ httpConfiguration.setSendServerVersion(false);
+ ConnectionFactory connectionFactory = new HttpConnectionFactory(httpConfiguration);
+ connector = new ServerConnector(server, connectionFactory);
+ }
+ connector.setPort(uri.getPort());
+ connector.setHost(uri.getHost());
+ connector.setName("Connector-" + i);
+ return connector;
}
public void internalStop() throws Exception {
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
Review Comment:
Resolved by moving this into `stop(boolean)`.
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -53,115 +54,122 @@
import org.eclipse.jetty.webapp.WebAppContext;
import org.jboss.logging.Logger;
-public class WebServerComponent implements ExternalComponent {
+public class WebServerComponent implements ExternalComponent, WebServerComponentMarker {
private static final Logger logger = Logger.getLogger(WebServerComponent.class);
+ public static final String DIR_ALLOWED = "org.eclipse.jetty.servlet.Default.dirAllowed";
private Server server;
private HandlerList handlers;
- private WebServerDTO webServerConfig;
+ private WebServerDTO config;
private final List<String> consoleUrls = new ArrayList<>();
private final List<String> jolokiaUrls = new ArrayList<>();
- private List<WebAppContext> webContexts;
+ private List<WebAppContext> webContexts = new ArrayList<>();;
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875214526
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -363,9 +366,17 @@ public void stop() throws Exception {
}
@Override
- public void stop(boolean isShutdown) throws Exception {
+ public synchronized void stop(boolean isShutdown) throws Exception {
if (isShutdown) {
- internalStop();
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
+ server.stop();
+ server = null;
Review Comment:
Done.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4295,101 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ long time = System.currentTimeMillis();
+ Assert.assertTrue(time >= fake.getStartTime());
+ Assert.assertTrue(time > fake.getStopTime());
+ Thread.sleep(5);
+ serverControl.restartEmbeddedWebServer();
+ Assert.assertTrue(serverControl.isEmbeddedWebServerStarted());
+ Assert.assertTrue(time < fake.getStartTime());
+ Assert.assertTrue(time < fake.getStopTime());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServerTimeout() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent(400);
+ server.addExternalComponent(fake, false);
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ try {
+ serverControl.restartEmbeddedWebServer(100);
Review Comment:
I _think_ I understood your point. Let me know if I didn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863090610
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java:
##########
@@ -1971,5 +1971,11 @@ void replay(@Parameter(name = "startScanDate", desc = "Start date where we will
@Parameter(name = "address", desc = "Name of the address to replay") String address,
@Parameter(name = "target", desc = "Where the replay data should be sent") String target,
@Parameter(name = "filter", desc = "Filter to apply on message selection. Null means everything matching the address") String filter) throws Exception;
+
+ @Operation(desc = "stop the embedded web server", impact = MBeanOperationInfo.ACTION)
+ void stopEmbeddedWebServer() throws Exception;
Review Comment:
can you add a method to restart the WebServer?
if you wanted to restart with a web console, how would you get it back running?
maybe restartEmbeddedWebServer(int timeoutSeconds);
Such method would stop it...
wait a few seconds.. and restart it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863108348
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java:
##########
@@ -1971,5 +1971,11 @@ void replay(@Parameter(name = "startScanDate", desc = "Start date where we will
@Parameter(name = "address", desc = "Name of the address to replay") String address,
@Parameter(name = "target", desc = "Where the replay data should be sent") String target,
@Parameter(name = "filter", desc = "Filter to apply on message selection. Null means everything matching the address") String filter) throws Exception;
+
+ @Operation(desc = "stop the embedded web server", impact = MBeanOperationInfo.ACTION)
+ void stopEmbeddedWebServer() throws Exception;
Review Comment:
@jbertram Yeah.. It would be nice (and I think simple) to have it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875999083
##########
artemis-website/package-lock.json:
##########
@@ -3676,7 +3676,7 @@
"dev": true,
"requires": {
"graceful-fs": "^4.1.5",
- "minimist": "^1.2.6",
+ "minimist": "^1.2.0",
Review Comment:
This was a mistake. Reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r869592574
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,65 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ started = true;
+ startTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public void stop() throws Exception {
+ }
+
+ @Override
+ public void stop(boolean shutdown) throws Exception {
+ started = false;
+ }
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#issuecomment-1115526289
@clebertsuconic looks like that was the culprit. Try now. In my testing it's reliable now from JConsole and the web console as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871717347
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -193,87 +203,97 @@ public void configure(ComponentDTO config, String artemisInstance, String artemi
instanceContext.setResourceBase(instanceWarDir.toString());
instanceContext.setHandler(instanceResourceHandler);
instanceContext.setVirtualHosts(virtualHosts);
- homeContext.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false");
+ homeContext.setInitParameter(DIR_ALLOWED, "false");
DefaultHandler defaultHandler = new DefaultHandler();
defaultHandler.setServeIcon(false);
- if (webServerConfig.requestLog != null) {
+ if (this.config.requestLog != null) {
handlers.addHandler(getLogHandler());
}
handlers.addHandler(homeContext);
handlers.addHandler(instanceContext);
handlers.addHandler(defaultHandler); // this should be last
server.setHandler(handlers);
- }
- private RequestLogHandler getLogHandler() {
- RequestLogWriter requestLogWriter = new RequestLogWriter();
- CustomRequestLog requestLog;
+ cleanupTmp();
+ server.start();
- // required via config so no check necessary
- requestLogWriter.setFilename(webServerConfig.requestLog.filename);
+ ActiveMQWebLogger.LOGGER.webserverStarted(bindings
+ .stream()
+ .map(binding -> binding.uri)
+ .collect(Collectors.joining(", ")));
- if (webServerConfig.requestLog.append != null) {
- requestLogWriter.setAppend(webServerConfig.requestLog.append);
- }
+ ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
+ ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ }
- if (webServerConfig.requestLog.filenameDateFormat != null) {
- requestLogWriter.setFilenameDateFormat(webServerConfig.requestLog.filenameDateFormat);
- }
+ private ServerConnector createServerConnector(HttpConfiguration httpConfiguration,
+ int i,
+ BindingDTO binding,
+ URI uri,
+ String scheme) throws Exception {
+ ServerConnector connector;
- if (webServerConfig.requestLog.retainDays != null) {
- requestLogWriter.setRetainDays(webServerConfig.requestLog.retainDays);
- }
+ if ("https".equals(scheme)) {
+ SslContextFactory.Server sslFactory = new SslContextFactory.Server();
+ sslFactory.setKeyStorePath(binding.keyStorePath == null ? artemisInstance + "/etc/keystore.jks" : binding.keyStorePath);
+ sslFactory.setKeyStorePassword(binding.getKeyStorePassword() == null ? "password" : binding.getKeyStorePassword());
- if (webServerConfig.requestLog.format != null) {
- requestLog = new CustomRequestLog(requestLogWriter, webServerConfig.requestLog.format);
- } else if (webServerConfig.requestLog.extended != null && webServerConfig.requestLog.extended) {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT);
- } else {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.NCSA_FORMAT);
- }
-
- if (webServerConfig.requestLog.ignorePaths != null && webServerConfig.requestLog.ignorePaths.length() > 0) {
- String[] split = webServerConfig.requestLog.ignorePaths.split(",");
- String[] ignorePaths = new String[split.length];
- for (int i = 0; i < ignorePaths.length; i++) {
- ignorePaths[i] = split[i].trim();
+ if (binding.getIncludedTLSProtocols() != null) {
+ sslFactory.setIncludeProtocols(binding.getIncludedTLSProtocols());
+ }
+ if (binding.getExcludedTLSProtocols() != null) {
+ sslFactory.setExcludeProtocols(binding.getExcludedTLSProtocols());
+ }
+ if (binding.getIncludedCipherSuites() != null) {
+ sslFactory.setIncludeCipherSuites(binding.getIncludedCipherSuites());
+ }
+ if (binding.getExcludedCipherSuites() != null) {
+ sslFactory.setExcludeCipherSuites(binding.getExcludedCipherSuites());
+ }
+ if (binding.clientAuth != null) {
+ sslFactory.setNeedClientAuth(binding.clientAuth);
+ if (binding.clientAuth) {
+ sslFactory.setTrustStorePath(binding.trustStorePath);
+ sslFactory.setTrustStorePassword(binding.getTrustStorePassword());
+ }
}
- requestLog.setIgnorePaths(ignorePaths);
- }
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- return requestLogHandler;
- }
+ SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- @Override
- public void start() throws Exception {
- if (isStarted()) {
- return;
- }
- cleanupTmp();
- server.start();
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ httpConfiguration.setSendServerVersion(false);
+ HttpConnectionFactory httpFactory = new HttpConnectionFactory(httpConfiguration);
- String bindings = webServerConfig.getBindings()
- .stream()
- .map(binding -> binding.uri)
- .collect(Collectors.joining(", "));
- ActiveMQWebLogger.LOGGER.webserverStarted(bindings);
+ connector = new ServerConnector(server, sslConnectionFactory, httpFactory);
- ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
- ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ } else {
+ httpConfiguration.setSendServerVersion(false);
+ ConnectionFactory connectionFactory = new HttpConnectionFactory(httpConfiguration);
+ connector = new ServerConnector(server, connectionFactory);
+ }
+ connector.setPort(uri.getPort());
+ connector.setHost(uri.getHost());
+ connector.setName("Connector-" + i);
+ return connector;
}
public void internalStop() throws Exception {
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
+ System.out.println("Stopping...");
server.stop();
+ server = null;
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r876099013
##########
artemis-website/package-lock.json:
##########
@@ -3734,7 +3734,7 @@
"boolbase": "~1.0.0",
"css-what": "2.1",
"domutils": "1.5.1",
- "nth-check": "^2.0.1"
+ "nth-check": "~1.0.1"
Review Comment:
This one seems unrelated also.
##########
artemis-website/package-lock.json:
##########
@@ -4899,8 +4899,7 @@
"dev": true
},
"nth-check": {
- "version": "2.0.1",
- "resolved": "https://registry.npmjs.org/nth-check/-/nth-check-2.0.1.tgz",
+ "version": "https://registry.npmjs.org/nth-check/-/nth-check-2.0.1.tgz",
Review Comment:
This one seems unrelated also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r876122818
##########
artemis-website/package-lock.json:
##########
@@ -4899,8 +4899,7 @@
"dev": true
},
"nth-check": {
- "version": "2.0.1",
- "resolved": "https://registry.npmjs.org/nth-check/-/nth-check-2.0.1.tgz",
+ "version": "https://registry.npmjs.org/nth-check/-/nth-check-2.0.1.tgz",
Review Comment:
Removed.
##########
artemis-website/package-lock.json:
##########
@@ -3734,7 +3734,7 @@
"boolbase": "~1.0.0",
"css-what": "2.1",
"domutils": "1.5.1",
- "nth-check": "^2.0.1"
+ "nth-check": "~1.0.1"
Review Comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r869187012
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4482,39 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ for (ActiveMQComponent component : server.getExternalComponents()) {
+ if (component instanceof WebServerComponentMarker) {
+ ((ServiceComponent) component).stop(true);
+ }
+ }
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ for (ActiveMQComponent component : server.getExternalComponents()) {
+ if (component instanceof WebServerComponentMarker) {
+ component.start();
+ }
+ }
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
Review Comment:
Its actually the bits around here calling start/stop or in this case stop+start that I thought should be coordinated for consistency, but changing the component itself was a good change too and takes care of some of the problems.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,65 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ started = true;
+ startTime = System.currentTimeMillis();
Review Comment:
Should probably gate this update on it not already being started, for a better validation it was first stopped.
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -175,31 +166,31 @@ public void testComponentStopBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
webServerComponent.configure(webServerDTO, "./src/test/resources/", "./src/test/resources/");
webServerComponent.start();
- final int port = webServerComponent.getPort();
// Make the connection attempt.
- CountDownLatch latch = new CountDownLatch(1);
- final ClientHandler clientHandler = new ClientHandler(latch);
- bootstrap.group(group).channel(NioSocketChannel.class).handler(new ChannelInitializer() {
- @Override
- protected void initChannel(Channel ch) throws Exception {
- ch.pipeline().addLast(new HttpClientCodec());
- ch.pipeline().addLast(clientHandler);
- }
- });
- Channel ch = bootstrap.connect("localhost", port).sync().channel();
+ verifyConnection(webServerComponent.getPort());
+ Assert.assertTrue(webServerComponent.isStarted());
- URI uri = new URI(URL);
- // Prepare the HTTP request.
- HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri.getRawPath());
- request.headers().set(HttpHeaderNames.HOST, "localhost");
+ //usual stop won't actually stop it
+ webServerComponent.stop();
+ assertTrue(webServerComponent.isStarted());
- // Send the HTTP request.
- ch.writeAndFlush(request);
- assertTrue(latch.await(5, TimeUnit.SECONDS));
- assertEquals("12345", clientHandler.body.toString());
- // Wait for the server to close the connection.
- ch.close();
- ch.eventLoop().shutdownNow();
+ webServerComponent.stop(true);
+ Assert.assertFalse(webServerComponent.isStarted());
+ }
+
+ @Test
+ public void testComponentStopStartBehavior() throws Exception {
Review Comment:
Thats partly my point. None of that stuff or it related docs seems to be changing here, i.e everythings just now magically 'covering this' powerful new restart-the-management stuff. Anyone not seeing the JIRA go past would be likely to not even know it [now] exists, which seems off. I'd guess some folks will want to restrict the stop/start/restart ability differently than some other operations if they knew it exists. Seems reasonable to test and especially document.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,65 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ started = true;
+ startTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public void stop() throws Exception {
+ }
+
+ @Override
+ public void stop(boolean shutdown) throws Exception {
+ started = false;
+ }
Review Comment:
Since its value is criticial to what happens in the real case, this Fake component should also ensure the boolean arg was as expected (i.e true) rather than assuming.
(Using e.g Mockito would probably have been smaller and allowed more granular verification of the calls made).
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,65 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ started = true;
+ startTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public void stop() throws Exception {
+ }
+
+ @Override
+ public void stop(boolean shutdown) throws Exception {
+ started = false;
+ }
+
+ @Override
+ public boolean isStarted() {
+ return started;
+ }
+
+ public long getStartTime() {
+ return startTime;
+ }
+ }
+ Fake fake = new Fake();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ if (manual) {
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ } else {
+ Assert.assertTrue(System.currentTimeMillis() > fake.getStartTime());
+ long time = System.currentTimeMillis();
+ Thread.sleep(1);
+ serverControl.restartEmbeddedWebServer();
+ Assert.assertTrue(time < fake.getStartTime());
Review Comment:
Isnt this racey if restart uses the executor, perhaps need a short Wait?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871717057
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4483,45 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().start();
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875759369
##########
artemis-website/package-lock.json:
##########
@@ -3676,7 +3676,7 @@
"dev": true,
"requires": {
"graceful-fs": "^4.1.5",
- "minimist": "^1.2.6",
+ "minimist": "^1.2.0",
Review Comment:
Seems like the changes in this file should go in separately if needed, they seem unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863923053
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4482,39 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ for (ActiveMQComponent component : server.getExternalComponents()) {
Review Comment:
Stopping the embedded web server from the web console is not a problem. It's having the thread survive so that it can _start_ it again that's the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#issuecomment-1115690368
I will test tomorrow.. (including on stopping from the console)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863953256
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQComponent.java:
##########
@@ -29,4 +31,8 @@ default void asyncStop(Runnable callback) throws Exception {
}
boolean isStarted();
+
+ default SimpleString getName() {
+ return SimpleString.toSimpleString("");
+ }
Review Comment:
Using String would seem nicer than expanding the use of SimpleString for something it doesnt seem needed for. It seemed like you had to change many existing instances of using String already.
Also the description says "This was necessary in order to actually find the WebServerComponent in the broker's list of components". Would a simple alternative be a marker interface, supplied from the broker and implemented by the base WebServerComponent class, allowing operating on it without needing any of the widespread String vs SimpleString changes or random-constant name based coordination ?
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ComponentConstants.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.utils;
+
+import org.apache.activemq.artemis.api.core.SimpleString;
+
+public final class ComponentConstants {
+
+ public static final SimpleString WEB_SERVER = RandomUtil.randomSimpleString();
Review Comment:
A random constant being used for coordination essentially inside the broker seems a bit icky.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4482,39 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ for (ActiveMQComponent component : server.getExternalComponents()) {
Review Comment:
The stop/start/restart seem like maybe they should be coordinated to avoid falling over each other.
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -175,31 +166,31 @@ public void testComponentStopBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
webServerComponent.configure(webServerDTO, "./src/test/resources/", "./src/test/resources/");
webServerComponent.start();
- final int port = webServerComponent.getPort();
// Make the connection attempt.
- CountDownLatch latch = new CountDownLatch(1);
- final ClientHandler clientHandler = new ClientHandler(latch);
- bootstrap.group(group).channel(NioSocketChannel.class).handler(new ChannelInitializer() {
- @Override
- protected void initChannel(Channel ch) throws Exception {
- ch.pipeline().addLast(new HttpClientCodec());
- ch.pipeline().addLast(clientHandler);
- }
- });
- Channel ch = bootstrap.connect("localhost", port).sync().channel();
+ verifyConnection(webServerComponent.getPort());
+ Assert.assertTrue(webServerComponent.isStarted());
- URI uri = new URI(URL);
- // Prepare the HTTP request.
- HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri.getRawPath());
- request.headers().set(HttpHeaderNames.HOST, "localhost");
+ //usual stop won't actually stop it
+ webServerComponent.stop();
+ assertTrue(webServerComponent.isStarted());
- // Send the HTTP request.
- ch.writeAndFlush(request);
- assertTrue(latch.await(5, TimeUnit.SECONDS));
- assertEquals("12345", clientHandler.body.toString());
- // Wait for the server to close the connection.
- ch.close();
- ch.eventLoop().shutdownNow();
+ webServerComponent.stop(true);
+ Assert.assertFalse(webServerComponent.isStarted());
+ }
+
+ @Test
+ public void testComponentStopStartBehavior() throws Exception {
Review Comment:
Perhaps worth some access control testing, given its likely uses of this will be restricted to certain people?. If these new operations could be covered implicitly in some way by existing controls it may also be good to spell out their addition in docs for upgraders so noone is caught out by them popping up. Perhaps even specific default config changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863094171
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java:
##########
@@ -1971,5 +1971,11 @@ void replay(@Parameter(name = "startScanDate", desc = "Start date where we will
@Parameter(name = "address", desc = "Name of the address to replay") String address,
@Parameter(name = "target", desc = "Where the replay data should be sent") String target,
@Parameter(name = "filter", desc = "Filter to apply on message selection. Null means everything matching the address") String filter) throws Exception;
+
+ @Operation(desc = "stop the embedded web server", impact = MBeanOperationInfo.ACTION)
+ void stopEmbeddedWebServer() throws Exception;
Review Comment:
With the current change, if you were using the web console to stop the embedded web server then you'd have to use something _other_ than the web console to start it again (e.g. JConsole, Jolokia, etc.). However, it does make sense to have a method to both stop & start it. The user experience would certainly be better with that method in most circumstances. I'll add that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871120191
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -193,87 +203,97 @@ public void configure(ComponentDTO config, String artemisInstance, String artemi
instanceContext.setResourceBase(instanceWarDir.toString());
instanceContext.setHandler(instanceResourceHandler);
instanceContext.setVirtualHosts(virtualHosts);
- homeContext.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false");
+ homeContext.setInitParameter(DIR_ALLOWED, "false");
DefaultHandler defaultHandler = new DefaultHandler();
defaultHandler.setServeIcon(false);
- if (webServerConfig.requestLog != null) {
+ if (this.config.requestLog != null) {
handlers.addHandler(getLogHandler());
}
handlers.addHandler(homeContext);
handlers.addHandler(instanceContext);
handlers.addHandler(defaultHandler); // this should be last
server.setHandler(handlers);
- }
- private RequestLogHandler getLogHandler() {
- RequestLogWriter requestLogWriter = new RequestLogWriter();
- CustomRequestLog requestLog;
+ cleanupTmp();
+ server.start();
- // required via config so no check necessary
- requestLogWriter.setFilename(webServerConfig.requestLog.filename);
+ ActiveMQWebLogger.LOGGER.webserverStarted(bindings
+ .stream()
+ .map(binding -> binding.uri)
+ .collect(Collectors.joining(", ")));
- if (webServerConfig.requestLog.append != null) {
- requestLogWriter.setAppend(webServerConfig.requestLog.append);
- }
+ ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
+ ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ }
- if (webServerConfig.requestLog.filenameDateFormat != null) {
- requestLogWriter.setFilenameDateFormat(webServerConfig.requestLog.filenameDateFormat);
- }
+ private ServerConnector createServerConnector(HttpConfiguration httpConfiguration,
+ int i,
+ BindingDTO binding,
+ URI uri,
+ String scheme) throws Exception {
+ ServerConnector connector;
- if (webServerConfig.requestLog.retainDays != null) {
- requestLogWriter.setRetainDays(webServerConfig.requestLog.retainDays);
- }
+ if ("https".equals(scheme)) {
+ SslContextFactory.Server sslFactory = new SslContextFactory.Server();
+ sslFactory.setKeyStorePath(binding.keyStorePath == null ? artemisInstance + "/etc/keystore.jks" : binding.keyStorePath);
+ sslFactory.setKeyStorePassword(binding.getKeyStorePassword() == null ? "password" : binding.getKeyStorePassword());
- if (webServerConfig.requestLog.format != null) {
- requestLog = new CustomRequestLog(requestLogWriter, webServerConfig.requestLog.format);
- } else if (webServerConfig.requestLog.extended != null && webServerConfig.requestLog.extended) {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT);
- } else {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.NCSA_FORMAT);
- }
-
- if (webServerConfig.requestLog.ignorePaths != null && webServerConfig.requestLog.ignorePaths.length() > 0) {
- String[] split = webServerConfig.requestLog.ignorePaths.split(",");
- String[] ignorePaths = new String[split.length];
- for (int i = 0; i < ignorePaths.length; i++) {
- ignorePaths[i] = split[i].trim();
+ if (binding.getIncludedTLSProtocols() != null) {
+ sslFactory.setIncludeProtocols(binding.getIncludedTLSProtocols());
+ }
+ if (binding.getExcludedTLSProtocols() != null) {
+ sslFactory.setExcludeProtocols(binding.getExcludedTLSProtocols());
+ }
+ if (binding.getIncludedCipherSuites() != null) {
+ sslFactory.setIncludeCipherSuites(binding.getIncludedCipherSuites());
+ }
+ if (binding.getExcludedCipherSuites() != null) {
+ sslFactory.setExcludeCipherSuites(binding.getExcludedCipherSuites());
+ }
+ if (binding.clientAuth != null) {
+ sslFactory.setNeedClientAuth(binding.clientAuth);
+ if (binding.clientAuth) {
+ sslFactory.setTrustStorePath(binding.trustStorePath);
+ sslFactory.setTrustStorePassword(binding.getTrustStorePassword());
+ }
}
- requestLog.setIgnorePaths(ignorePaths);
- }
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- return requestLogHandler;
- }
+ SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- @Override
- public void start() throws Exception {
- if (isStarted()) {
- return;
- }
- cleanupTmp();
- server.start();
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ httpConfiguration.setSendServerVersion(false);
+ HttpConnectionFactory httpFactory = new HttpConnectionFactory(httpConfiguration);
- String bindings = webServerConfig.getBindings()
- .stream()
- .map(binding -> binding.uri)
- .collect(Collectors.joining(", "));
- ActiveMQWebLogger.LOGGER.webserverStarted(bindings);
+ connector = new ServerConnector(server, sslConnectionFactory, httpFactory);
- ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
- ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ } else {
+ httpConfiguration.setSendServerVersion(false);
+ ConnectionFactory connectionFactory = new HttpConnectionFactory(httpConfiguration);
+ connector = new ServerConnector(server, connectionFactory);
+ }
+ connector.setPort(uri.getPort());
+ connector.setHost(uri.getHost());
+ connector.setName("Connector-" + i);
+ return connector;
}
public void internalStop() throws Exception {
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
+ System.out.println("Stopping...");
Review Comment:
Leftover System.out.println ?
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -53,115 +54,122 @@
import org.eclipse.jetty.webapp.WebAppContext;
import org.jboss.logging.Logger;
-public class WebServerComponent implements ExternalComponent {
+public class WebServerComponent implements ExternalComponent, WebServerComponentMarker {
private static final Logger logger = Logger.getLogger(WebServerComponent.class);
+ public static final String DIR_ALLOWED = "org.eclipse.jetty.servlet.Default.dirAllowed";
private Server server;
private HandlerList handlers;
- private WebServerDTO webServerConfig;
+ private WebServerDTO config;
private final List<String> consoleUrls = new ArrayList<>();
private final List<String> jolokiaUrls = new ArrayList<>();
- private List<WebAppContext> webContexts;
+ private List<WebAppContext> webContexts = new ArrayList<>();;
Review Comment:
Could it also be final if it is never nulled/recreated now.
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -193,87 +203,97 @@ public void configure(ComponentDTO config, String artemisInstance, String artemi
instanceContext.setResourceBase(instanceWarDir.toString());
instanceContext.setHandler(instanceResourceHandler);
instanceContext.setVirtualHosts(virtualHosts);
- homeContext.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false");
+ homeContext.setInitParameter(DIR_ALLOWED, "false");
DefaultHandler defaultHandler = new DefaultHandler();
defaultHandler.setServeIcon(false);
- if (webServerConfig.requestLog != null) {
+ if (this.config.requestLog != null) {
handlers.addHandler(getLogHandler());
}
handlers.addHandler(homeContext);
handlers.addHandler(instanceContext);
handlers.addHandler(defaultHandler); // this should be last
server.setHandler(handlers);
- }
- private RequestLogHandler getLogHandler() {
- RequestLogWriter requestLogWriter = new RequestLogWriter();
- CustomRequestLog requestLog;
+ cleanupTmp();
+ server.start();
- // required via config so no check necessary
- requestLogWriter.setFilename(webServerConfig.requestLog.filename);
+ ActiveMQWebLogger.LOGGER.webserverStarted(bindings
+ .stream()
+ .map(binding -> binding.uri)
+ .collect(Collectors.joining(", ")));
- if (webServerConfig.requestLog.append != null) {
- requestLogWriter.setAppend(webServerConfig.requestLog.append);
- }
+ ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
+ ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ }
- if (webServerConfig.requestLog.filenameDateFormat != null) {
- requestLogWriter.setFilenameDateFormat(webServerConfig.requestLog.filenameDateFormat);
- }
+ private ServerConnector createServerConnector(HttpConfiguration httpConfiguration,
+ int i,
+ BindingDTO binding,
+ URI uri,
+ String scheme) throws Exception {
+ ServerConnector connector;
- if (webServerConfig.requestLog.retainDays != null) {
- requestLogWriter.setRetainDays(webServerConfig.requestLog.retainDays);
- }
+ if ("https".equals(scheme)) {
+ SslContextFactory.Server sslFactory = new SslContextFactory.Server();
+ sslFactory.setKeyStorePath(binding.keyStorePath == null ? artemisInstance + "/etc/keystore.jks" : binding.keyStorePath);
+ sslFactory.setKeyStorePassword(binding.getKeyStorePassword() == null ? "password" : binding.getKeyStorePassword());
- if (webServerConfig.requestLog.format != null) {
- requestLog = new CustomRequestLog(requestLogWriter, webServerConfig.requestLog.format);
- } else if (webServerConfig.requestLog.extended != null && webServerConfig.requestLog.extended) {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT);
- } else {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.NCSA_FORMAT);
- }
-
- if (webServerConfig.requestLog.ignorePaths != null && webServerConfig.requestLog.ignorePaths.length() > 0) {
- String[] split = webServerConfig.requestLog.ignorePaths.split(",");
- String[] ignorePaths = new String[split.length];
- for (int i = 0; i < ignorePaths.length; i++) {
- ignorePaths[i] = split[i].trim();
+ if (binding.getIncludedTLSProtocols() != null) {
+ sslFactory.setIncludeProtocols(binding.getIncludedTLSProtocols());
+ }
+ if (binding.getExcludedTLSProtocols() != null) {
+ sslFactory.setExcludeProtocols(binding.getExcludedTLSProtocols());
+ }
+ if (binding.getIncludedCipherSuites() != null) {
+ sslFactory.setIncludeCipherSuites(binding.getIncludedCipherSuites());
+ }
+ if (binding.getExcludedCipherSuites() != null) {
+ sslFactory.setExcludeCipherSuites(binding.getExcludedCipherSuites());
+ }
+ if (binding.clientAuth != null) {
+ sslFactory.setNeedClientAuth(binding.clientAuth);
+ if (binding.clientAuth) {
+ sslFactory.setTrustStorePath(binding.trustStorePath);
+ sslFactory.setTrustStorePassword(binding.getTrustStorePassword());
+ }
}
- requestLog.setIgnorePaths(ignorePaths);
- }
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- return requestLogHandler;
- }
+ SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- @Override
- public void start() throws Exception {
- if (isStarted()) {
- return;
- }
- cleanupTmp();
- server.start();
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ httpConfiguration.setSendServerVersion(false);
+ HttpConnectionFactory httpFactory = new HttpConnectionFactory(httpConfiguration);
- String bindings = webServerConfig.getBindings()
- .stream()
- .map(binding -> binding.uri)
- .collect(Collectors.joining(", "));
- ActiveMQWebLogger.LOGGER.webserverStarted(bindings);
+ connector = new ServerConnector(server, sslConnectionFactory, httpFactory);
- ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
- ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ } else {
+ httpConfiguration.setSendServerVersion(false);
+ ConnectionFactory connectionFactory = new HttpConnectionFactory(httpConfiguration);
+ connector = new ServerConnector(server, connectionFactory);
+ }
+ connector.setPort(uri.getPort());
+ connector.setHost(uri.getHost());
+ connector.setName("Connector-" + i);
+ return connector;
}
public void internalStop() throws Exception {
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
+ System.out.println("Stopping...");
server.stop();
+ server = null;
Review Comment:
This makes isStarted() vulnerable to NPE as it uses the value twice and isnt synchronized. It should either be synchronized also or changed to capture it once.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4483,45 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().start();
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
+ } catch (Exception e) {
+ ActiveMQServerLogger.LOGGER.failedToRestartEmbeddedWebServer(e);
+ }
+ });
+ }
+
+ @Override
+ public boolean isEmbeddedWebServerStarted() throws Exception {
+ return getEmbeddedWebServerComponent().isStarted();
+ }
Review Comment:
Its not clear to me this one should throw rather than just return false, and possibly trace log in the case it couldnt find the server.
My thinking is, are there potential uses of this simple boolean property method that could still reasonably occur even without the web server, e.g this seems to be an MBean so could this be used without explicit invocation by a user via the JMX management even if running without it, which seems ok as it isnt actually part of the broker, and returning false then still seems reasonable?
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,80 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+ long stopTime = 0;
Review Comment:
As these are being used by different threads they should be volatile or their use synchronized like the real component.
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -53,115 +54,122 @@
import org.eclipse.jetty.webapp.WebAppContext;
import org.jboss.logging.Logger;
-public class WebServerComponent implements ExternalComponent {
+public class WebServerComponent implements ExternalComponent, WebServerComponentMarker {
private static final Logger logger = Logger.getLogger(WebServerComponent.class);
+ public static final String DIR_ALLOWED = "org.eclipse.jetty.servlet.Default.dirAllowed";
private Server server;
private HandlerList handlers;
- private WebServerDTO webServerConfig;
+ private WebServerDTO config;
private final List<String> consoleUrls = new ArrayList<>();
private final List<String> jolokiaUrls = new ArrayList<>();
- private List<WebAppContext> webContexts;
+ private List<WebAppContext> webContexts = new ArrayList<>();;
private ServerConnector[] connectors;
private Path artemisHomePath;
private Path temporaryWarDir;
+ private String artemisInstance;
+ private String artemisHome;
@Override
public void configure(ComponentDTO config, String artemisInstance, String artemisHome) throws Exception {
- webServerConfig = (WebServerDTO) config;
- server = new Server();
-
- HttpConfiguration httpConfiguration = new HttpConfiguration();
+ this.config = (WebServerDTO) config;
+ this.artemisInstance = artemisInstance;
+ this.artemisHome = artemisHome;
- if (webServerConfig.customizer != null) {
- try {
- httpConfiguration.addCustomizer((HttpConfiguration.Customizer) Class.forName(webServerConfig.customizer).getConstructor().newInstance());
- } catch (Throwable t) {
- ActiveMQWebLogger.LOGGER.customizerNotLoaded(webServerConfig.customizer, t);
- }
+ temporaryWarDir = Paths.get(artemisInstance != null ? artemisInstance : ".").resolve("tmp").resolve("webapps").toAbsolutePath();
+ if (!Files.exists(temporaryWarDir)) {
+ Files.createDirectories(temporaryWarDir);
}
+ }
- List<BindingDTO> bindings = webServerConfig.getBindings();
- connectors = new ServerConnector[bindings.size()];
- String[] virtualHosts = new String[bindings.size()];
-
- for (int i = 0; i < bindings.size(); i++) {
- BindingDTO binding = bindings.get(i);
- URI uri = new URI(binding.uri);
- String scheme = uri.getScheme();
- ServerConnector connector;
+ private RequestLogHandler getLogHandler() {
Review Comment:
The diff seems like it could perhaps be a smaller and more easily understandable if this methods ~40 lines hadnt all moved at the same time as the other 'actual changes' in the same area...does it actually need to?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4483,45 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().start();
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
Review Comment:
Thinking about this more I still think these need coordinated here in some way. If I call restart, and then I immediately call stop from the same thread, the webserver should end up stopped. As this is now, because they are handled in different threads it seems just as likely it will end up [re]started instead.
Regardless of approach taken to addressing that, it feels like maybe it should even also await the operation being completed for some period before returning. It could even throw on failure like start and stop would.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,80 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+ long stopTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ if (started) {
+ return;
+ }
+ started = true;
+ startTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public void stop() throws Exception {
+ }
+
+ @Override
+ public void stop(boolean shutdown) throws Exception {
+ if (!shutdown) {
+ throw new RuntimeException("shutdown flag must be true");
+ }
+ started = false;
+ stopTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public boolean isStarted() {
+ return started;
+ }
+
+ public long getStartTime() {
+ return startTime;
+ }
+
+ public long getStopTime() {
+ return startTime;
+ }
+ }
+ Fake fake = new Fake();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ if (manual) {
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ } else {
+ // this is technically async so need to do some gymnastics to test effectively
+ long time = System.currentTimeMillis();
+ Thread.sleep(5);
+ Assert.assertTrue(time > fake.getStartTime());
+ Assert.assertTrue(time > fake.getStopTime());
+ serverControl.restartEmbeddedWebServer();
+ Wait.assertTrue(() -> time < fake.getStopTime(), 5000, 100);
+ Wait.assertTrue(() -> serverControl.isEmbeddedWebServerStarted(), 5000, 100);
Review Comment:
Shouldnt there be a tiny sleep before getting currentTimeMillis, helping ensure the 'time' is definitely later than the prior startTime. Its likely to be already, but doesnt seem guaranteed. The second sleep would then seem better placed just before the restart is called since its there to influence the times from that.
Alternatively, rather than having any sleeps, you could just reset the Fake components variable to 0 and just track they got updated by it changing from 0 again.
The 100ms recheck period seems quite slow. Unless its known to be glacially slow I'd go with something more granular, e.g 10ms.
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -193,87 +203,97 @@ public void configure(ComponentDTO config, String artemisInstance, String artemi
instanceContext.setResourceBase(instanceWarDir.toString());
instanceContext.setHandler(instanceResourceHandler);
instanceContext.setVirtualHosts(virtualHosts);
- homeContext.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false");
+ homeContext.setInitParameter(DIR_ALLOWED, "false");
DefaultHandler defaultHandler = new DefaultHandler();
defaultHandler.setServeIcon(false);
- if (webServerConfig.requestLog != null) {
+ if (this.config.requestLog != null) {
handlers.addHandler(getLogHandler());
}
handlers.addHandler(homeContext);
handlers.addHandler(instanceContext);
handlers.addHandler(defaultHandler); // this should be last
server.setHandler(handlers);
- }
- private RequestLogHandler getLogHandler() {
- RequestLogWriter requestLogWriter = new RequestLogWriter();
- CustomRequestLog requestLog;
+ cleanupTmp();
+ server.start();
- // required via config so no check necessary
- requestLogWriter.setFilename(webServerConfig.requestLog.filename);
+ ActiveMQWebLogger.LOGGER.webserverStarted(bindings
+ .stream()
+ .map(binding -> binding.uri)
+ .collect(Collectors.joining(", ")));
- if (webServerConfig.requestLog.append != null) {
- requestLogWriter.setAppend(webServerConfig.requestLog.append);
- }
+ ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
+ ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ }
- if (webServerConfig.requestLog.filenameDateFormat != null) {
- requestLogWriter.setFilenameDateFormat(webServerConfig.requestLog.filenameDateFormat);
- }
+ private ServerConnector createServerConnector(HttpConfiguration httpConfiguration,
+ int i,
+ BindingDTO binding,
+ URI uri,
+ String scheme) throws Exception {
+ ServerConnector connector;
- if (webServerConfig.requestLog.retainDays != null) {
- requestLogWriter.setRetainDays(webServerConfig.requestLog.retainDays);
- }
+ if ("https".equals(scheme)) {
+ SslContextFactory.Server sslFactory = new SslContextFactory.Server();
+ sslFactory.setKeyStorePath(binding.keyStorePath == null ? artemisInstance + "/etc/keystore.jks" : binding.keyStorePath);
+ sslFactory.setKeyStorePassword(binding.getKeyStorePassword() == null ? "password" : binding.getKeyStorePassword());
- if (webServerConfig.requestLog.format != null) {
- requestLog = new CustomRequestLog(requestLogWriter, webServerConfig.requestLog.format);
- } else if (webServerConfig.requestLog.extended != null && webServerConfig.requestLog.extended) {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT);
- } else {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.NCSA_FORMAT);
- }
-
- if (webServerConfig.requestLog.ignorePaths != null && webServerConfig.requestLog.ignorePaths.length() > 0) {
- String[] split = webServerConfig.requestLog.ignorePaths.split(",");
- String[] ignorePaths = new String[split.length];
- for (int i = 0; i < ignorePaths.length; i++) {
- ignorePaths[i] = split[i].trim();
+ if (binding.getIncludedTLSProtocols() != null) {
+ sslFactory.setIncludeProtocols(binding.getIncludedTLSProtocols());
+ }
+ if (binding.getExcludedTLSProtocols() != null) {
+ sslFactory.setExcludeProtocols(binding.getExcludedTLSProtocols());
+ }
+ if (binding.getIncludedCipherSuites() != null) {
+ sslFactory.setIncludeCipherSuites(binding.getIncludedCipherSuites());
+ }
+ if (binding.getExcludedCipherSuites() != null) {
+ sslFactory.setExcludeCipherSuites(binding.getExcludedCipherSuites());
+ }
+ if (binding.clientAuth != null) {
+ sslFactory.setNeedClientAuth(binding.clientAuth);
+ if (binding.clientAuth) {
+ sslFactory.setTrustStorePath(binding.trustStorePath);
+ sslFactory.setTrustStorePassword(binding.getTrustStorePassword());
+ }
}
- requestLog.setIgnorePaths(ignorePaths);
- }
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- return requestLogHandler;
- }
+ SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- @Override
- public void start() throws Exception {
- if (isStarted()) {
- return;
- }
- cleanupTmp();
- server.start();
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ httpConfiguration.setSendServerVersion(false);
+ HttpConnectionFactory httpFactory = new HttpConnectionFactory(httpConfiguration);
- String bindings = webServerConfig.getBindings()
- .stream()
- .map(binding -> binding.uri)
- .collect(Collectors.joining(", "));
- ActiveMQWebLogger.LOGGER.webserverStarted(bindings);
+ connector = new ServerConnector(server, sslConnectionFactory, httpFactory);
- ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
- ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ } else {
+ httpConfiguration.setSendServerVersion(false);
+ ConnectionFactory connectionFactory = new HttpConnectionFactory(httpConfiguration);
+ connector = new ServerConnector(server, connectionFactory);
+ }
+ connector.setPort(uri.getPort());
+ connector.setHost(uri.getHost());
+ connector.setName("Connector-" + i);
+ return connector;
}
public void internalStop() throws Exception {
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
Review Comment:
This comment is actually for the line above the logger, the internalStop() method seems like it should be private.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875214854
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4487,68 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().start();
+ }
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ restartEmbeddedWebServer(ActiveMQDefaultConfiguration.getDefaultEmbeddedWebServerRestartTimeout());
+ }
+
+ @Override
+ public void restartEmbeddedWebServer(long timeout) throws Exception {
+ final CountDownLatch latch = new CountDownLatch(1);
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ synchronized (embeddedWebServerLock) {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
+ }
+ latch.countDown();
+ } catch (Exception e) {
+ ActiveMQServerLogger.LOGGER.failedToRestartEmbeddedWebServer(e);
+ }
+ });
+ if (!latch.await(timeout, TimeUnit.MILLISECONDS)) {
+ throw ActiveMQMessageBundle.BUNDLE.embeddedWebServerNotRestarted(timeout);
+ }
Review Comment:
Should be resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871719745
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4483,45 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ getEmbeddedWebServerComponent().start();
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
+ } catch (Exception e) {
+ ActiveMQServerLogger.LOGGER.failedToRestartEmbeddedWebServer(e);
+ }
+ });
+ }
+
+ @Override
+ public boolean isEmbeddedWebServerStarted() throws Exception {
+ return getEmbeddedWebServerComponent().isStarted();
+ }
Review Comment:
Agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875998489
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4488,71 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().start();
+ }
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ restartEmbeddedWebServer(ActiveMQDefaultConfiguration.getDefaultEmbeddedWebServerRestartTimeout());
+ }
+
+ @Override
+ public void restartEmbeddedWebServer(long timeout) throws Exception {
+ final CountDownLatch latch = new CountDownLatch(1);
+ final AtomicReference<Exception> exception = new AtomicReference<>();
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ synchronized (embeddedWebServerLock) {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
+ }
+ } catch (Exception e) {
+ exception.set(e);
+ } finally {
+ latch.countDown();
+ }
+ });
+ boolean result = !latch.await(timeout, TimeUnit.MILLISECONDS);
+ if (result || exception.get() != null) {
+ throw ActiveMQMessageBundle.BUNDLE.embeddedWebServerNotRestarted(timeout, exception.get());
+ }
Review Comment:
Yes, that would be more clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r868286574
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -175,31 +166,31 @@ public void testComponentStopBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
webServerComponent.configure(webServerDTO, "./src/test/resources/", "./src/test/resources/");
webServerComponent.start();
- final int port = webServerComponent.getPort();
// Make the connection attempt.
- CountDownLatch latch = new CountDownLatch(1);
- final ClientHandler clientHandler = new ClientHandler(latch);
- bootstrap.group(group).channel(NioSocketChannel.class).handler(new ChannelInitializer() {
- @Override
- protected void initChannel(Channel ch) throws Exception {
- ch.pipeline().addLast(new HttpClientCodec());
- ch.pipeline().addLast(clientHandler);
- }
- });
- Channel ch = bootstrap.connect("localhost", port).sync().channel();
+ verifyConnection(webServerComponent.getPort());
+ Assert.assertTrue(webServerComponent.isStarted());
- URI uri = new URI(URL);
- // Prepare the HTTP request.
- HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri.getRawPath());
- request.headers().set(HttpHeaderNames.HOST, "localhost");
+ //usual stop won't actually stop it
+ webServerComponent.stop();
+ assertTrue(webServerComponent.isStarted());
- // Send the HTTP request.
- ch.writeAndFlush(request);
- assertTrue(latch.await(5, TimeUnit.SECONDS));
- assertEquals("12345", clientHandler.body.toString());
- // Wait for the server to close the connection.
- ch.close();
- ch.eventLoop().shutdownNow();
+ webServerComponent.stop(true);
+ Assert.assertFalse(webServerComponent.isStarted());
+ }
+
+ @Test
+ public void testComponentStopStartBehavior() throws Exception {
Review Comment:
The default config in `management.xml` should cover these methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r869592309
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,65 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ started = true;
+ startTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public void stop() throws Exception {
+ }
+
+ @Override
+ public void stop(boolean shutdown) throws Exception {
+ started = false;
+ }
+
+ @Override
+ public boolean isStarted() {
+ return started;
+ }
+
+ public long getStartTime() {
+ return startTime;
+ }
+ }
+ Fake fake = new Fake();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ if (manual) {
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ } else {
+ Assert.assertTrue(System.currentTimeMillis() > fake.getStartTime());
+ long time = System.currentTimeMillis();
+ Thread.sleep(1);
+ serverControl.restartEmbeddedWebServer();
+ Assert.assertTrue(time < fake.getStartTime());
Review Comment:
Good point.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,65 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ started = true;
+ startTime = System.currentTimeMillis();
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r869592999
##########
artemis-web/src/test/java/org/apache/activemq/cli/test/WebServerComponentTest.java:
##########
@@ -175,31 +166,31 @@ public void testComponentStopBehavior() throws Exception {
Assert.assertFalse(webServerComponent.isStarted());
webServerComponent.configure(webServerDTO, "./src/test/resources/", "./src/test/resources/");
webServerComponent.start();
- final int port = webServerComponent.getPort();
// Make the connection attempt.
- CountDownLatch latch = new CountDownLatch(1);
- final ClientHandler clientHandler = new ClientHandler(latch);
- bootstrap.group(group).channel(NioSocketChannel.class).handler(new ChannelInitializer() {
- @Override
- protected void initChannel(Channel ch) throws Exception {
- ch.pipeline().addLast(new HttpClientCodec());
- ch.pipeline().addLast(clientHandler);
- }
- });
- Channel ch = bootstrap.connect("localhost", port).sync().channel();
+ verifyConnection(webServerComponent.getPort());
+ Assert.assertTrue(webServerComponent.isStarted());
- URI uri = new URI(URL);
- // Prepare the HTTP request.
- HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri.getRawPath());
- request.headers().set(HttpHeaderNames.HOST, "localhost");
+ //usual stop won't actually stop it
+ webServerComponent.stop();
+ assertTrue(webServerComponent.isStarted());
- // Send the HTTP request.
- ch.writeAndFlush(request);
- assertTrue(latch.await(5, TimeUnit.SECONDS));
- assertEquals("12345", clientHandler.body.toString());
- // Wait for the server to close the connection.
- ch.close();
- ch.eventLoop().shutdownNow();
+ webServerComponent.stop(true);
+ Assert.assertFalse(webServerComponent.isStarted());
+ }
+
+ @Test
+ public void testComponentStopStartBehavior() throws Exception {
Review Comment:
I added docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#issuecomment-1115479957
If I run `restartEmbeddedWebServer` from JConsole everything works just fine. However, if I run it from the web console it doesn't work. The web server never restarts. I think the problem is that the thread servicing the request from the web console is controlled by Jetty and since Jetty is shutting down the thread just halts. It's a catch-22. I'll try running it in its own thread and see if that works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863437999
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4482,39 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ for (ActiveMQComponent component : server.getExternalComponents()) {
Review Comment:
shouldn't this be on its own thread as well? what if you stop from the console?
don't make sense I know, but wouldn't be broken if a user do it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r863136705
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/core/management/ActiveMQServerControl.java:
##########
@@ -1971,5 +1971,11 @@ void replay(@Parameter(name = "startScanDate", desc = "Start date where we will
@Parameter(name = "address", desc = "Name of the address to replay") String address,
@Parameter(name = "target", desc = "Where the replay data should be sent") String target,
@Parameter(name = "filter", desc = "Filter to apply on message selection. Null means everything matching the address") String filter) throws Exception;
+
+ @Operation(desc = "stop the embedded web server", impact = MBeanOperationInfo.ACTION)
+ void stopEmbeddedWebServer() throws Exception;
Review Comment:
@clebertsuconic done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r868284271
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4482,39 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ for (ActiveMQComponent component : server.getExternalComponents()) {
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871717940
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -193,87 +203,97 @@ public void configure(ComponentDTO config, String artemisInstance, String artemi
instanceContext.setResourceBase(instanceWarDir.toString());
instanceContext.setHandler(instanceResourceHandler);
instanceContext.setVirtualHosts(virtualHosts);
- homeContext.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed", "false");
+ homeContext.setInitParameter(DIR_ALLOWED, "false");
DefaultHandler defaultHandler = new DefaultHandler();
defaultHandler.setServeIcon(false);
- if (webServerConfig.requestLog != null) {
+ if (this.config.requestLog != null) {
handlers.addHandler(getLogHandler());
}
handlers.addHandler(homeContext);
handlers.addHandler(instanceContext);
handlers.addHandler(defaultHandler); // this should be last
server.setHandler(handlers);
- }
- private RequestLogHandler getLogHandler() {
- RequestLogWriter requestLogWriter = new RequestLogWriter();
- CustomRequestLog requestLog;
+ cleanupTmp();
+ server.start();
- // required via config so no check necessary
- requestLogWriter.setFilename(webServerConfig.requestLog.filename);
+ ActiveMQWebLogger.LOGGER.webserverStarted(bindings
+ .stream()
+ .map(binding -> binding.uri)
+ .collect(Collectors.joining(", ")));
- if (webServerConfig.requestLog.append != null) {
- requestLogWriter.setAppend(webServerConfig.requestLog.append);
- }
+ ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
+ ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ }
- if (webServerConfig.requestLog.filenameDateFormat != null) {
- requestLogWriter.setFilenameDateFormat(webServerConfig.requestLog.filenameDateFormat);
- }
+ private ServerConnector createServerConnector(HttpConfiguration httpConfiguration,
+ int i,
+ BindingDTO binding,
+ URI uri,
+ String scheme) throws Exception {
+ ServerConnector connector;
- if (webServerConfig.requestLog.retainDays != null) {
- requestLogWriter.setRetainDays(webServerConfig.requestLog.retainDays);
- }
+ if ("https".equals(scheme)) {
+ SslContextFactory.Server sslFactory = new SslContextFactory.Server();
+ sslFactory.setKeyStorePath(binding.keyStorePath == null ? artemisInstance + "/etc/keystore.jks" : binding.keyStorePath);
+ sslFactory.setKeyStorePassword(binding.getKeyStorePassword() == null ? "password" : binding.getKeyStorePassword());
- if (webServerConfig.requestLog.format != null) {
- requestLog = new CustomRequestLog(requestLogWriter, webServerConfig.requestLog.format);
- } else if (webServerConfig.requestLog.extended != null && webServerConfig.requestLog.extended) {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.EXTENDED_NCSA_FORMAT);
- } else {
- requestLog = new CustomRequestLog(requestLogWriter, CustomRequestLog.NCSA_FORMAT);
- }
-
- if (webServerConfig.requestLog.ignorePaths != null && webServerConfig.requestLog.ignorePaths.length() > 0) {
- String[] split = webServerConfig.requestLog.ignorePaths.split(",");
- String[] ignorePaths = new String[split.length];
- for (int i = 0; i < ignorePaths.length; i++) {
- ignorePaths[i] = split[i].trim();
+ if (binding.getIncludedTLSProtocols() != null) {
+ sslFactory.setIncludeProtocols(binding.getIncludedTLSProtocols());
+ }
+ if (binding.getExcludedTLSProtocols() != null) {
+ sslFactory.setExcludeProtocols(binding.getExcludedTLSProtocols());
+ }
+ if (binding.getIncludedCipherSuites() != null) {
+ sslFactory.setIncludeCipherSuites(binding.getIncludedCipherSuites());
+ }
+ if (binding.getExcludedCipherSuites() != null) {
+ sslFactory.setExcludeCipherSuites(binding.getExcludedCipherSuites());
+ }
+ if (binding.clientAuth != null) {
+ sslFactory.setNeedClientAuth(binding.clientAuth);
+ if (binding.clientAuth) {
+ sslFactory.setTrustStorePath(binding.trustStorePath);
+ sslFactory.setTrustStorePassword(binding.getTrustStorePassword());
+ }
}
- requestLog.setIgnorePaths(ignorePaths);
- }
- RequestLogHandler requestLogHandler = new RequestLogHandler();
- requestLogHandler.setRequestLog(requestLog);
- return requestLogHandler;
- }
+ SslConnectionFactory sslConnectionFactory = new SslConnectionFactory(sslFactory, "HTTP/1.1");
- @Override
- public void start() throws Exception {
- if (isStarted()) {
- return;
- }
- cleanupTmp();
- server.start();
+ httpConfiguration.addCustomizer(new SecureRequestCustomizer());
+ httpConfiguration.setSendServerVersion(false);
+ HttpConnectionFactory httpFactory = new HttpConnectionFactory(httpConfiguration);
- String bindings = webServerConfig.getBindings()
- .stream()
- .map(binding -> binding.uri)
- .collect(Collectors.joining(", "));
- ActiveMQWebLogger.LOGGER.webserverStarted(bindings);
+ connector = new ServerConnector(server, sslConnectionFactory, httpFactory);
- ActiveMQWebLogger.LOGGER.jolokiaAvailable(String.join(", ", jolokiaUrls));
- ActiveMQWebLogger.LOGGER.consoleAvailable(String.join(", ", consoleUrls));
+ } else {
+ httpConfiguration.setSendServerVersion(false);
+ ConnectionFactory connectionFactory = new HttpConnectionFactory(httpConfiguration);
+ connector = new ServerConnector(server, connectionFactory);
+ }
+ connector.setPort(uri.getPort());
+ connector.setHost(uri.getHost());
+ connector.setName("Connector-" + i);
+ return connector;
}
public void internalStop() throws Exception {
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
+ System.out.println("Stopping...");
Review Comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871719618
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,80 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+ long stopTime = 0;
+
+ @Override
+ public void start() throws Exception {
+ if (started) {
+ return;
+ }
+ started = true;
+ startTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public void stop() throws Exception {
+ }
+
+ @Override
+ public void stop(boolean shutdown) throws Exception {
+ if (!shutdown) {
+ throw new RuntimeException("shutdown flag must be true");
+ }
+ started = false;
+ stopTime = System.currentTimeMillis();
+ }
+
+ @Override
+ public boolean isStarted() {
+ return started;
+ }
+
+ public long getStartTime() {
+ return startTime;
+ }
+
+ public long getStopTime() {
+ return startTime;
+ }
+ }
+ Fake fake = new Fake();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ if (manual) {
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ } else {
+ // this is technically async so need to do some gymnastics to test effectively
+ long time = System.currentTimeMillis();
+ Thread.sleep(5);
+ Assert.assertTrue(time > fake.getStartTime());
+ Assert.assertTrue(time > fake.getStopTime());
+ serverControl.restartEmbeddedWebServer();
+ Wait.assertTrue(() -> time < fake.getStopTime(), 5000, 100);
+ Wait.assertTrue(() -> serverControl.isEmbeddedWebServerStarted(), 5000, 100);
Review Comment:
Should be resolved now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871718776
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4292,80 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(true);
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ testStartStopEmbeddedWebServer(false);
+ }
+
+ private void testStartStopEmbeddedWebServer(boolean manual) throws Exception {
+ class Fake implements ServiceComponent, WebServerComponentMarker {
+ boolean started = false;
+ long startTime = 0;
+ long stopTime = 0;
Review Comment:
Resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
URL: https://github.com/apache/activemq-artemis/pull/4061
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r871718590
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -53,115 +54,122 @@
import org.eclipse.jetty.webapp.WebAppContext;
import org.jboss.logging.Logger;
-public class WebServerComponent implements ExternalComponent {
+public class WebServerComponent implements ExternalComponent, WebServerComponentMarker {
private static final Logger logger = Logger.getLogger(WebServerComponent.class);
+ public static final String DIR_ALLOWED = "org.eclipse.jetty.servlet.Default.dirAllowed";
private Server server;
private HandlerList handlers;
- private WebServerDTO webServerConfig;
+ private WebServerDTO config;
private final List<String> consoleUrls = new ArrayList<>();
private final List<String> jolokiaUrls = new ArrayList<>();
- private List<WebAppContext> webContexts;
+ private List<WebAppContext> webContexts = new ArrayList<>();;
private ServerConnector[] connectors;
private Path artemisHomePath;
private Path temporaryWarDir;
+ private String artemisInstance;
+ private String artemisHome;
@Override
public void configure(ComponentDTO config, String artemisInstance, String artemisHome) throws Exception {
- webServerConfig = (WebServerDTO) config;
- server = new Server();
-
- HttpConfiguration httpConfiguration = new HttpConfiguration();
+ this.config = (WebServerDTO) config;
+ this.artemisInstance = artemisInstance;
+ this.artemisHome = artemisHome;
- if (webServerConfig.customizer != null) {
- try {
- httpConfiguration.addCustomizer((HttpConfiguration.Customizer) Class.forName(webServerConfig.customizer).getConstructor().newInstance());
- } catch (Throwable t) {
- ActiveMQWebLogger.LOGGER.customizerNotLoaded(webServerConfig.customizer, t);
- }
+ temporaryWarDir = Paths.get(artemisInstance != null ? artemisInstance : ".").resolve("tmp").resolve("webapps").toAbsolutePath();
+ if (!Files.exists(temporaryWarDir)) {
+ Files.createDirectories(temporaryWarDir);
}
+ }
- List<BindingDTO> bindings = webServerConfig.getBindings();
- connectors = new ServerConnector[bindings.size()];
- String[] virtualHosts = new String[bindings.size()];
-
- for (int i = 0; i < bindings.size(); i++) {
- BindingDTO binding = bindings.get(i);
- URI uri = new URI(binding.uri);
- String scheme = uri.getScheme();
- ServerConnector connector;
+ private RequestLogHandler getLogHandler() {
Review Comment:
I think this is mainly due to the way the diff algorithm works, but I rearranged some stuff to eliminate it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r872187352
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4487,68 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().start();
+ }
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ restartEmbeddedWebServer(ActiveMQDefaultConfiguration.getDefaultEmbeddedWebServerRestartTimeout());
+ }
+
+ @Override
+ public void restartEmbeddedWebServer(long timeout) throws Exception {
+ final CountDownLatch latch = new CountDownLatch(1);
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ synchronized (embeddedWebServerLock) {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
+ }
+ latch.countDown();
+ } catch (Exception e) {
+ ActiveMQServerLogger.LOGGER.failedToRestartEmbeddedWebServer(e);
+ }
+ });
+ if (!latch.await(timeout, TimeUnit.MILLISECONDS)) {
+ throw ActiveMQMessageBundle.BUNDLE.embeddedWebServerNotRestarted(timeout);
+ }
Review Comment:
This handles the waiting for completion, but misses the other aspect around throwing the failure when the restarts start or stop fails. Right now it would catch and log a failure on the executor, but the caller would then just sit there for 5 seconds (or other given value) and time out even though it is already known it failed and wont complete. It should just fail immediately, tripping the latch and throwing based on the exception its currently logging.
You could add a simple mechanism to pass any exception back...or I see that ActiveMQServer#getThreadPool() returns Executor but ActiveMQServerImpl#getThreadPool() actually returns ExecutorService. If the interface could be expanded perhaps then you could use submit/Callable for the waiting and exception handling.
##########
artemis-web/src/main/java/org/apache/activemq/artemis/component/WebServerComponent.java:
##########
@@ -363,9 +366,17 @@ public void stop() throws Exception {
}
@Override
- public void stop(boolean isShutdown) throws Exception {
+ public synchronized void stop(boolean isShutdown) throws Exception {
if (isShutdown) {
- internalStop();
+ ActiveMQWebLogger.LOGGER.stoppingEmbeddedWebServer();
+ server.stop();
+ server = null;
Review Comment:
This method should check that it is running first before it gets to doing this, much like start() does. It would currently NPE if you called stop(true) while it is already stopped, as e.g restart does, since _server_ would be null but this assumes it is populated.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4295,101 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ long time = System.currentTimeMillis();
+ Assert.assertTrue(time >= fake.getStartTime());
+ Assert.assertTrue(time > fake.getStopTime());
+ Thread.sleep(5);
+ serverControl.restartEmbeddedWebServer();
+ Assert.assertTrue(serverControl.isEmbeddedWebServerStarted());
+ Assert.assertTrue(time < fake.getStartTime());
+ Assert.assertTrue(time < fake.getStopTime());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServerTimeout() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent(400);
+ server.addExternalComponent(fake, false);
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ try {
+ serverControl.restartEmbeddedWebServer(100);
Review Comment:
Seems unnecessary to wait 100ms for timeout when the point is to timeout and the other thread is only waiting 400ms to complete/record starting, might as well make it something smaller. It would make the test quicker and less likely to fail (by accidentally succeeding) on a busy system due to the whims of thread scheduling and only having 300ms between the two values.
You could shorten the test and make it more reliable by having the FakeWebServerComponent await on a latch you pass it instead, rather than passing a small fixed sleep time. You can then give it something longer to wait for to help ensure it does cause a timeout, and then trip it to complete from the main test immediately after the restart call has actually timed out. Avoids the other thread needing to dangling after the core test is actually finished, and is more reliable in various ways (e.g right now its also susceptible to spurious wakeups on its Thread.sleep() whereas the latch would handle the wait properly).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#issuecomment-1115357313
@jbertram I did some simple testing.. and the console would not work after a restart:
```
022-05-02 16:52:16,600 INFO [org.apache.activemq.hawtio.branding.PluginContextListener] Destroyed activemq-branding plugin
2022-05-02 16:52:31,616 INFO [io.hawt.web.auth.AuthenticationFilter] Failed to invoke action /exec/org.apache.activemq.artemis:broker="0.0.0.0"/restartEmbeddedWebServer() due to:: java.security.PrivilegedActionException: org.eclipse.jetty.io.EofException
at java.base/java.security.AccessController.doPrivileged(Native Method) [java.base:]
at java.base/javax.security.auth.Subject.doAs(Subject.java:423) [java.base:]
at io.hawt.web.auth.AuthenticationFilter.executeAs(AuthenticationFilter.java:104) [hawtio-system-2.14.2.jar:2.14.2]
at io.hawt.web.auth.AuthenticationFilter.doFilter(AuthenticationFilter.java:72) [hawtio-system-2.14.2.jar:2.14.2]
at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
at io.hawt.web.filters.HttpHeaderFilter.doFilter(HttpHeaderFilter.java:43) [hawtio-system-2.14.2.jar:2.14.2]
at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
at io.hawt.web.filters.HttpHeaderFilter.doFilter(HttpHeaderFilter.java:43) [hawtio-system-2.14.2.jar:2.14.2]
at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
at io.hawt.web.filters.HttpHeaderFilter.doFilter(HttpHeaderFilter.java:43) [hawtio-system-2.14.2.jar:2.14.2]
at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
at io.hawt.web.filters.HttpHeaderFilter.doFilter(HttpHeaderFilter.java:43) [hawtio-system-2.14.2.jar:2.14.2]
at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
at io.hawt.web.filters.HttpHeaderFilter.doFilter(HttpHeaderFilter.java:43) [hawtio-system-2.14.2.jar:2.14.2]
at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193)
at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601)
at io.hawt.web.filters.HttpHeaderFilter.doFilter(HttpHeaderFilter.java:43) [hawtio-system-2.14.2.jar:2.14.2]
```
I don't think this is related to the restart method. I think there's something else happening there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875998731
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4296,106 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ long time = System.currentTimeMillis();
+ Assert.assertTrue(time >= fake.getStartTime());
+ Assert.assertTrue(time > fake.getStopTime());
+ Thread.sleep(5);
+ serverControl.restartEmbeddedWebServer();
+ Assert.assertTrue(serverControl.isEmbeddedWebServerStarted());
+ Assert.assertTrue(time < fake.getStartTime());
+ Assert.assertTrue(time < fake.getStopTime());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServerTimeout() throws Exception {
+ final CountDownLatch startDelay = new CountDownLatch(1);
+ FakeWebServerComponent fake = new FakeWebServerComponent(startDelay);
+ server.addExternalComponent(fake, false);
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ try {
+ serverControl.restartEmbeddedWebServer(10);
+ fail();
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof ActiveMQIllegalStateException);
+ }
+ startDelay.countDown();
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875999445
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java:
##########
@@ -522,4 +522,10 @@ IllegalStateException invalidRoutingTypeUpdate(String queueName,
@Message(id = 229240, value = "Connection router {0} rejected the connection", format = Message.Format.MESSAGE_FORMAT)
ActiveMQRemoteDisconnectException connectionRejected(String connectionRouter);
+
+ @Message(id = 229241, value = "Embedded web server not found")
+ ActiveMQIllegalStateException embeddedWebServerNotFound();
+
+ @Message(id = 229242, value = "Embedded web server not restarted in {0} milliseconds", format = Message.Format.MESSAGE_FORMAT)
+ ActiveMQIllegalStateException embeddedWebServerNotRestarted(long timeout, @Cause Exception e);
Review Comment:
Fair enough. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r875734514
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java:
##########
@@ -4479,5 +4488,71 @@ public void replay(String startScan, String endScan, String address, String targ
server.replay(startScanDate, endScanDate, address, target, filter);
}
+
+ @Override
+ public void stopEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().stop(true);
+ }
+ }
+
+ @Override
+ public void startEmbeddedWebServer() throws Exception {
+ synchronized (embeddedWebServerLock) {
+ getEmbeddedWebServerComponent().start();
+ }
+ }
+
+ @Override
+ public void restartEmbeddedWebServer() throws Exception {
+ restartEmbeddedWebServer(ActiveMQDefaultConfiguration.getDefaultEmbeddedWebServerRestartTimeout());
+ }
+
+ @Override
+ public void restartEmbeddedWebServer(long timeout) throws Exception {
+ final CountDownLatch latch = new CountDownLatch(1);
+ final AtomicReference<Exception> exception = new AtomicReference<>();
+ /*
+ * This needs to be run in its own thread managed by the broker because if it is run on a thread managed by Jetty
+ * (e.g. if it is invoked from the web console) then the thread will die before Jetty can be restarted.
+ */
+ server.getThreadPool().execute(() -> {
+ try {
+ synchronized (embeddedWebServerLock) {
+ stopEmbeddedWebServer();
+ startEmbeddedWebServer();
+ }
+ } catch (Exception e) {
+ exception.set(e);
+ } finally {
+ latch.countDown();
+ }
+ });
+ boolean result = !latch.await(timeout, TimeUnit.MILLISECONDS);
+ if (result || exception.get() != null) {
+ throw ActiveMQMessageBundle.BUNDLE.embeddedWebServerNotRestarted(timeout, exception.get());
+ }
Review Comment:
```suggestion
if (!latch.await(timeout, TimeUnit.MILLISECONDS)) {
throw ActiveMQMessageBundle.BUNDLE.embeddedWebServerRestartTimeout(timeout);
} else if (exception.get() != null) {
throw ActiveMQMessageBundle.BUNDLE.embeddedWebServerRestartFailed(exception.get());
}
```
Could make the failure mode and exceptions clearer if they were reported separately: either the execution timed out and the state isnt actually known (it may even execute later on just fine), or it failed by throwing while we waited for execution.
(ActiveMQMessageBundle obviously would need changed to match the suggestion change)
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java:
##########
@@ -522,4 +522,10 @@ IllegalStateException invalidRoutingTypeUpdate(String queueName,
@Message(id = 229240, value = "Connection router {0} rejected the connection", format = Message.Format.MESSAGE_FORMAT)
ActiveMQRemoteDisconnectException connectionRejected(String connectionRouter);
+
+ @Message(id = 229241, value = "Embedded web server not found")
+ ActiveMQIllegalStateException embeddedWebServerNotFound();
+
+ @Message(id = 229242, value = "Embedded web server not restarted in {0} milliseconds", format = Message.Format.MESSAGE_FORMAT)
+ ActiveMQIllegalStateException embeddedWebServerNotRestarted(long timeout, @Cause Exception e);
Review Comment:
Using ActiveMQIllegalStateException doesnt seem quite right. Its not that the user tried to do something when it was in the wrong state, its that a result wasnt known in an allotted time, so some kind of timeout exception woudl be more appropriate.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/ActiveMQServerControlTest.java:
##########
@@ -4290,6 +4296,106 @@ public boolean isStarted() {
}
}
+ @Test
+ public void testManualStopStartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ serverControl.stopEmbeddedWebServer();
+ Assert.assertFalse(fake.isStarted());
+ serverControl.startEmbeddedWebServer();
+ Assert.assertTrue(fake.isStarted());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServer() throws Exception {
+ FakeWebServerComponent fake = new FakeWebServerComponent();
+ server.addExternalComponent(fake, true);
+ Assert.assertTrue(fake.isStarted());
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ long time = System.currentTimeMillis();
+ Assert.assertTrue(time >= fake.getStartTime());
+ Assert.assertTrue(time > fake.getStopTime());
+ Thread.sleep(5);
+ serverControl.restartEmbeddedWebServer();
+ Assert.assertTrue(serverControl.isEmbeddedWebServerStarted());
+ Assert.assertTrue(time < fake.getStartTime());
+ Assert.assertTrue(time < fake.getStopTime());
+ }
+
+ @Test
+ public void testRestartEmbeddedWebServerTimeout() throws Exception {
+ final CountDownLatch startDelay = new CountDownLatch(1);
+ FakeWebServerComponent fake = new FakeWebServerComponent(startDelay);
+ server.addExternalComponent(fake, false);
+
+ ActiveMQServerControl serverControl = createManagementControl();
+ try {
+ serverControl.restartEmbeddedWebServer(10);
+ fail();
+ } catch (Exception e) {
+ Assert.assertTrue(e instanceof ActiveMQIllegalStateException);
+ }
+ startDelay.countDown();
Review Comment:
The countdown should go in a finally or it might never happen. Rather than catching Exception it could just catch the expected exception type and let everything else bubble up (then you also dont need the assertion).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4061: ARTEMIS-3808 support start/stop embedded web server via mngmnt
Posted by GitBox <gi...@apache.org>.
jbertram commented on code in PR #4061:
URL: https://github.com/apache/activemq-artemis/pull/4061#discussion_r868244998
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQComponent.java:
##########
@@ -29,4 +31,8 @@ default void asyncStop(Runnable callback) throws Exception {
}
boolean isStarted();
+
+ default SimpleString getName() {
+ return SimpleString.toSimpleString("");
+ }
Review Comment:
I think using a marker interface is a great idea. I hadn't considered that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org