[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314348659
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng2305MultipleProxiesTest.java
 ##
 @@ -71,20 +78,16 @@ public void testit()
 String keyPwd = "key-passwd";
 
 Server server = new Server( 0 );
-server.addConnector( newHttpsConnector( storePath, storePwd, keyPwd ) 
);
+addHttpsConnector( server, storePath, storePwd, keyPwd );
 server.setHandler( new RepoHandler() );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 {
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
 }
-int httpPort = server.getConnectors()[0].getLocalPort();
+int httpPort = ( (NetworkConnector) server.getConnectors()[0] 
).getLocalPort();
 System.out.println( "Bound server socket to HTTP port " + httpPort );
-int httpsPort = server.getConnectors()[1].getLocalPort();
+int httpsPort = ( (NetworkConnector) server.getConnectors()[1] 
).getLocalPort();
 
 Review comment:
   Then your Requests never indicate that they are secure.
   The Requests will not contain the security details.
   The Requests will not have the javax attributes for the SSL / Certificate 
state of the connection.
   Nor will you have ability to use the "confidential" constraints.
   And usages that redirect from HTTP to HTTPS will also fail. (such as 
SecureRedirectHandler)


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314347605
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0553SettingsAuthzEncryptionTest.java
 ##
 @@ -86,18 +93,13 @@ protected void setUp()
 handlerList.addHandler( repoHandler );
 handlerList.addHandler( new DefaultHandler() );
 
-server = new Server( 0 );
 server.setHandler( handlerList );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 
 Review comment:
   If you have parallel processes sure.
   But your usages are sequential.
   If you decided to use something like DeploymentManager to scan for webapps 
and deploy them, then you would be interested in ThrowUnavailableOnStartup, 
which waits for initial deployment to report failures to Server.start


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314287943
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0768OfflineModeTest.java
 ##
 @@ -103,17 +103,12 @@ else if ( request.getRequestURI().endsWith( ".md5" ) || 
request.getRequestURI().
 try
 {
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 
 Review comment:
   This line is not possible to reach immediately after `server.start()` in a 
Bind failure scenario.
   
   The ServerConnector's doStart will attempt to bind
   The BindException gets thrown for failure to bind
   A IOException wraps failure to bind during ServerConnector doStart
   The Server doStart captures the IOException and adds it to the 
MultiException its tracking
   At the end of Server doStart the MultiException throws if it has any 
exceptions its tracking.
   
   This scenario should result in a IOException or MultiException from 
server.start()
   


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314301663
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0553SettingsAuthzEncryptionTest.java
 ##
 @@ -86,18 +93,13 @@ protected void setUp()
 handlerList.addHandler( repoHandler );
 handlerList.addHandler( new DefaultHandler() );
 
-server = new Server( 0 );
 server.setHandler( handlerList );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 
 Review comment:
   That is the generic `start()` for `AbstractLifeCycle`.
   `Server` is a `ContainerLifeCycle` and a special one at that.
   It's `Server.doStart()` has to do far more then this abstract base behavior 
simply because it's a combination of a Container (with child lifecycles) and 
it's the main entry point for external that care about the LifeCycle state.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314284534
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
 ##
 @@ -59,15 +60,11 @@ protected void setUp()
 server = new Server( 0 );
 server.setHandler( handlers );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 {
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
 }
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
 
 Review comment:
   There's also `server.getURI().getPort()` as a valid option here.
   (This will return the URI to the first connector's security scheme (http vs 
https) + local host + port + first handler's own context path)
   


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314288543
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng2305MultipleProxiesTest.java
 ##
 @@ -71,20 +78,16 @@ public void testit()
 String keyPwd = "key-passwd";
 
 Server server = new Server( 0 );
-server.addConnector( newHttpsConnector( storePath, storePwd, keyPwd ) 
);
+addHttpsConnector( server, storePath, storePwd, keyPwd );
 server.setHandler( new RepoHandler() );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 {
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
 }
-int httpPort = server.getConnectors()[0].getLocalPort();
+int httpPort = ( (NetworkConnector) server.getConnectors()[0] 
).getLocalPort();
 System.out.println( "Bound server socket to HTTP port " + httpPort );
-int httpsPort = server.getConnectors()[1].getLocalPort();
+int httpsPort = ( (NetworkConnector) server.getConnectors()[1] 
).getLocalPort();
 
 Review comment:
   The `HttpConfiguration` belonging to Connector[0] needs to have its 
`HttpConfiguration.setSecurePort(int)` set for the port that this HTTPS 
connector.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314287943
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0768OfflineModeTest.java
 ##
 @@ -103,17 +103,12 @@ else if ( request.getRequestURI().endsWith( ".md5" ) || 
request.getRequestURI().
 try
 {
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 
 Review comment:
   This state is not possible to reach immediately after `server.start()` in a 
Bind failure scenario.
   
   The ServerConnector's doStart will attempt to bind
   The BindException gets thrown for failure to bind
   A IOException wraps failure to bind during ServerConnector doStart
   The Server doStart captures the IOException and adds it to the 
MultiException its tracking
   At the end of Server doStart the MultiException throws if it has any 
exceptions its tracking.
   
   This scenario should result in a IOException or MultiException from 
server.start()
   


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314284936
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenITmng0553SettingsAuthzEncryptionTest.java
 ##
 @@ -86,18 +93,13 @@ protected void setUp()
 handlerList.addHandler( repoHandler );
 handlerList.addHandler( new DefaultHandler() );
 
-server = new Server( 0 );
 server.setHandler( handlerList );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 
 Review comment:
   Server could have successfully started up, and then be successfully stopped, 
before this line hits.
   In this scenario, the server LifeCycle state is STOPPED, not FAILED.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314284534
 
 

 ##
 File path: 
core-it-suite/src/test/java/org/apache/maven/it/MavenIT0146InstallerSnapshotNaming.java
 ##
 @@ -59,15 +60,11 @@ protected void setUp()
 server = new Server( 0 );
 server.setHandler( handlers );
 server.start();
-while ( !server.isRunning() || !server.isStarted() )
+if ( server.isFailed() )
 {
-if ( server.isFailed() )
-{
-fail( "Couldn't bind the server socket to a free port!" );
-}
-Thread.sleep( 100L );
+fail( "Couldn't bind the server socket to a free port!" );
 }
-port = server.getConnectors()[0].getLocalPort();
+port = ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
 
 Review comment:
   There's also `server.getURI().getPort()` as a valid option here.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [maven-integration-testing] joakime commented on a change in pull request #46: [MNG-6731] Jetty getLocalPort() returns -1 resulting in build failures

[GitBox]

joakime commented on a change in pull request #46: [MNG-6731] Jetty 
getLocalPort() returns -1 resulting in build failures
URL: 
https://github.com/apache/maven-integration-testing/pull/46#discussion_r314284271
 
 

 ##
 File path: core-it-suite/src/test/java/org/apache/maven/it/HttpServer.java
 ##
 @@ -66,21 +68,20 @@ public void stop()
 
 public int port()
 {
-return ( (ServerConnector) server.getConnectors()[0] ).getLocalPort();
+return ( (NetworkConnector) server.getConnectors()[0] ).getLocalPort();
 }
 
 private Server server( int port )
 {
-
 QueuedThreadPool threadPool = new QueuedThreadPool();
 threadPool.setMaxThreads( 500 );
 Server server = new Server( threadPool );
+server.setConnectors( new Connector[]{ new ServerConnector( server ) } 
);
 server.addBean( new ScheduledExecutorScheduler() );
 
-ServerConnector http = new ServerConnector( server );
-http.setPort( port );
-http.setIdleTimeout( 3 );
-server.addConnector( http );
+ServerConnector connector = (ServerConnector) 
server.getConnectors()[0];
+connector.setIdleTimeout( 30_000L );
+connector.setPort( port );
 
 Review comment:
   Why not just do this instead ?
   
   ``` java
   ServerConnector connector = new ServerConnector( server );
   connector.setIdleTimeout( 30_000L );
   connector.setPort( port );
   server.addConnector( connector );
   ```


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services