tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: svn commit: r1484923 - /tomcat/tc6.0.x/trunk/java/org/apache/catalina/startup/ContextConfig.java
Date Tue, 21 May 2013 22:10:30 GMT
2013/5/22  <markt@apache.org>:
> Author: markt
> Date: Tue May 21 20:01:02 2013
> New Revision: 1484923
>
> URL: http://svn.apache.org/r1484923
> Log:
> Make deletion of the copied WARs used for anti-resource locking more robust if the context
fails to start (there were some circumstances where the original WAR could get deleted).
>
> Modified:
>     tomcat/tc6.0.x/trunk/java/org/apache/catalina/startup/ContextConfig.java
>
> Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/startup/ContextConfig.java
> URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/startup/ContextConfig.java?rev=1484923&r1=1484922&r2=1484923&view=diff
> ==============================================================================
> --- tomcat/tc6.0.x/trunk/java/org/apache/catalina/startup/ContextConfig.java (original)
> +++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/startup/ContextConfig.java Tue May
21 20:01:02 2013
> @@ -127,6 +127,12 @@ public class ContextConfig
>
>
>      /**
> +     * Anti-locking docBase
> +     */
> +    private String antiLockingDocBase = null;
> +
> +
> +    /**
>       * The string resources for this package.
>       */
>      protected static final StringManager sm =
> @@ -264,16 +270,9 @@ public class ContextConfig
>          } else if (event.getType().equals(StandardContext.AFTER_START_EVENT)) {
>              // Restore docBase for management tools
>              if (originalDocBase != null) {
> -                String docBase = context.getDocBase();
>                  context.setDocBase(originalDocBase);
> -                originalDocBase = docBase;
>              }
>          } else if (event.getType().equals(Lifecycle.STOP_EVENT)) {
> -            if (originalDocBase != null) {
> -                String docBase = context.getDocBase();
> -                context.setDocBase(originalDocBase);
> -                originalDocBase = docBase;
> -            }
>              stop();
>          } else if (event.getType().equals(Lifecycle.INIT_EVENT)) {
>              init();
> @@ -931,8 +930,7 @@ public class ContextConfig
>      }
>
>
> -    protected void antiLocking()
> -        throws IOException {
> +    protected void antiLocking() throws IOException {
>
>          if ((context instanceof StandardContext)
>              && ((StandardContext) context).getAntiResourceLocking()) {
> @@ -942,11 +940,8 @@ public class ContextConfig
>              String docBase = context.getDocBase();
>              if (docBase == null)
>                  return;
> -            if (originalDocBase == null) {
> -                originalDocBase = docBase;
> -            } else {
> -                docBase = originalDocBase;
> -            }
> +            originalDocBase = docBase;
> +

2. Why if(originalDocBase == null) check was removed?


>              File docBaseFile = new File(docBase);
>              if (!docBaseFile.isAbsolute()) {
>                  File file = new File(appBase);
> @@ -983,14 +978,13 @@ public class ContextConfig
>                  log.debug("Anti locking context[" + context.getPath()
>                          + "] setting docBase to " + file);
>
> +            antiLockingDocBase = file.getAbsolutePath();
>              // Cleanup just in case an old deployment is lying around
>              ExpandWar.delete(file);
>              if (ExpandWar.copy(docBaseFile, file)) {
> -                context.setDocBase(file.getAbsolutePath());
> +                context.setDocBase(antiLockingDocBase);
>              }
> -
>          }
> -
>      }
>
>
> @@ -1264,8 +1258,8 @@ public class ContextConfig
>          Host host = (Host) context.getParent();
>          String appBase = host.getAppBase();
>          String docBase = context.getDocBase();
> -        if ((docBase != null) && (originalDocBase != null)) {
> -            File docBaseFile = new File(docBase);
> +        if (antiLockingDocBase != null) {
> +            File docBaseFile = new File(antiLockingDocBase);
>              if (!docBaseFile.isAbsolute()) {
>                  docBaseFile = new File(appBase, docBase);

1. This change in 6.0 needs to go through voting
2. There is a bug in the above line,
"docBase" should not be there.
(Though it never executes, as antiLockingDocBase was created as
file.getAbsolutePath(),)
It will allow to simplify this block a bit.

3. Wouldn't it be more simple to have the new field as a File instead of String?
While "docBase" is a String, per API,  this field represents a proper file.

The rest of the change is OK.

(It changes the way how "originalDocBase" field is handled, but I
would say that it is OK. The new behaviour makes more sense. Regarding
lifecycleEvent() method, now it goes as:

a) BEFORE_START_EVENT -> beforeStart()
 -> call antiLocking() -> save docBase into originalDocBase field

b) START_EVENT -> (...)

c) AFTER_START_EVENT -> restore docBase

Thus there was OK to remove that "restoring" code from STOP_EVENT block.
)


>              }
>
>

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message