Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1
    • Fix Version/s: 2.0
    • Component/s: File reloading
    • Labels:
      None

      Description

      Current reloading process clears current properties and load updated values from
      resource reader. If an IO error occurs (or invalid format), the configuration
      gets corrupted and the application becomes unstable.

      It may be better for hot-reload to put loaded values into a temporary Properties
      and replace previous values only when reloading is successful.

      It may also allow to use a 'currentlty-reloading' flag in the synchronized
      'reload' block to avoid blocking threads during a reload (they could access
      safelly the 'old' properties until reload is finished)

      1. commons-configuration-1.5_patch_CONFIGURATION-136.jar
        273 kB
        Laurent Michelet
      2. commons-configuration-1.8_patch_CONFIGURATION-136.jar
        322 kB
        Laurent Michelet
      3. patch.txt
        3 kB
        Laurent Michelet

        Issue Links

          Activity

          Hide
          Emmanuel Bourg added a comment - - edited

          This issue has been on the todo list for quite some time. It could be quickly
          fixed by changing the reload() method in AbstractFileConfiguration, it may look
          like this :

              public void reload()
              {
                  synchronized (reloadLock)
                  {
                      if (noReload == 0 && strategy.reloadingRequired())
                      {
                          // save the properties
                          Map previousProperties = new LinkedMap(store);
          
                          try
                          {
                              clear();
                              load();
          
                              // notify the strategy
                              strategy.reloadingPerformed();
                          }
                          catch (Exception e)
                          {
                              e.printStackTrace();
          
                              // rollback the changes
                              store = previousProperties;
                          }
                      }
                  }
              }
          

          I'm not sure it's the best way to rollback the changes though, copying the
          properties seems expensive. Any idea ? Should we add a safeReload flag to
          enable/disable this feature ?

          Show
          Emmanuel Bourg added a comment - - edited This issue has been on the todo list for quite some time. It could be quickly fixed by changing the reload() method in AbstractFileConfiguration, it may look like this : public void reload() { synchronized (reloadLock) { if (noReload == 0 && strategy.reloadingRequired()) { // save the properties Map previousProperties = new LinkedMap(store); try { clear(); load(); // notify the strategy strategy.reloadingPerformed(); } catch (Exception e) { e.printStackTrace(); // rollback the changes store = previousProperties; } } } } I'm not sure it's the best way to rollback the changes though, copying the properties seems expensive. Any idea ? Should we add a safeReload flag to enable/disable this feature ?
          Hide
          Emmanuel Bourg added a comment -

          The copy of the store can be avoided by keeping a reference to the previous store and creating a new empty store before loading the file :

          Map previousStore = store;
          store = new LinkedMap();

          However this will only work for the "flat" configurations like PropertiesConfiguration and INIConfiguration, the hierarchical configurations like XMLConfiguration don't use the Map in BaseConfiguration to store the properties.

          Show
          Emmanuel Bourg added a comment - The copy of the store can be avoided by keeping a reference to the previous store and creating a new empty store before loading the file : Map previousStore = store; store = new LinkedMap(); However this will only work for the "flat" configurations like PropertiesConfiguration and INIConfiguration, the hierarchical configurations like XMLConfiguration don't use the Map in BaseConfiguration to store the properties.
          Hide
          Laurent Michelet added a comment - - edited

          THe problem here is that filesystem is not transactional. When an exception is raised, there is no rollback on the file which is written.

          A proposal of correction is to save in memory the file before any modifications. When an exception is raised we replace the current file with the original one.

          The 2 following patches has been tested successfully with XMLConfiguration file.

          AbstractFileConfiguration.java
          
          	/**
          	 * Save the configuration to the specified URL. This doesn't change the
          	 * source of the configuration, use setURL() if you need it.
          	 * 
          	 * @param url
          	 *            the URL
          	 * 
          	 * @throws ConfigurationException
          	 *             if an error occurs during the save operation
          	 */
          	public void save(URL url) throws ConfigurationException {
          		OutputStream out = null;
          		ByteArrayInputStream originalFile = null;
          		try {
          			final File file = new File(url.getFile());
          			// Save original file if existing
          			if (file.exists()) {
          				InputStream inputStreamOfOrignalFile = fileSystem
          						.getInputStream(url);
          				originalFile = saveOriginalFile(inputStreamOfOrignalFile);
          			}
          			out = fileSystem.getOutputStream(url);
          			save(out);
          			if (out instanceof VerifiableOutputStream) {
          				((VerifiableOutputStream) out).verify();
          			}
          		} catch (IOException e) {
          			// Rollback for IOException if existing
          			if (originalFile != null) {
          				reloadOriginalFile(url, originalFile, out);
          			}
          			throw new ConfigurationException("Could not save to URL " + url, e);
          		} catch (Exception e) {
          			// Rollback when any kind of Exception is raised if existing
          			if (originalFile != null) {
          				reloadOriginalFile(url, originalFile, out);
          			}
          			throw new ConfigurationException(e);
          		} finally {
          			closeSilent(out);
          		}
          	}
          
          	/**
          	 * Save the original file before any modifications
          	 * 
          	 * @param in
          	 * @return
          	 * @throws IOException
          	 * @since 1.9
          	 */
          	private ByteArrayInputStream saveOriginalFile(
          			InputStream inputStreamOfOrignalFile) throws IOException {
          		ByteArrayOutputStream baos = new ByteArrayOutputStream();
          		byte[] buffer = new byte[1024];
          		int len;
          		while ((len = inputStreamOfOrignalFile.read(buffer)) > -1) {
          			baos.write(buffer, 0, len);
          		}
          		baos.flush();
          		return new ByteArrayInputStream(baos.toByteArray());
          	}
          
          	/**
          	 * Replace the current file with the original one before any modifications
          	 * 
          	 * @param url
          	 * @param originalFile
          	 * @param outOfCurrentFile
          	 * @throws ConfigurationException
          	 * @since 1.9
          	 */
          	private void reloadOriginalFile(URL url, ByteArrayInputStream originalFile,
          			OutputStream outOfCurrentFile) throws ConfigurationException {
          		if (outOfCurrentFile != null && originalFile != null) {
          			try {
          				int nextChar;
          				while ((nextChar = originalFile.read()) != -1)
          					outOfCurrentFile.write((char) nextChar);
          				outOfCurrentFile.write('\n');
          				outOfCurrentFile.flush();
          			} catch (IOException ioe) {
          				throw new ConfigurationException(
          						"Could not save to URL " + url, ioe);
          			}
          		}
          	}
          
          
          Show
          Laurent Michelet added a comment - - edited THe problem here is that filesystem is not transactional. When an exception is raised, there is no rollback on the file which is written. A proposal of correction is to save in memory the file before any modifications. When an exception is raised we replace the current file with the original one. The 2 following patches has been tested successfully with XMLConfiguration file. AbstractFileConfiguration.java /** * Save the configuration to the specified URL. This doesn't change the * source of the configuration, use setURL() if you need it. * * @param url * the URL * * @ throws ConfigurationException * if an error occurs during the save operation */ public void save(URL url) throws ConfigurationException { OutputStream out = null ; ByteArrayInputStream originalFile = null ; try { final File file = new File(url.getFile()); // Save original file if existing if (file.exists()) { InputStream inputStreamOfOrignalFile = fileSystem .getInputStream(url); originalFile = saveOriginalFile(inputStreamOfOrignalFile); } out = fileSystem.getOutputStream(url); save(out); if (out instanceof VerifiableOutputStream) { ((VerifiableOutputStream) out).verify(); } } catch (IOException e) { // Rollback for IOException if existing if (originalFile != null ) { reloadOriginalFile(url, originalFile, out); } throw new ConfigurationException( "Could not save to URL " + url, e); } catch (Exception e) { // Rollback when any kind of Exception is raised if existing if (originalFile != null ) { reloadOriginalFile(url, originalFile, out); } throw new ConfigurationException(e); } finally { closeSilent(out); } } /** * Save the original file before any modifications * * @param in * @ return * @ throws IOException * @since 1.9 */ private ByteArrayInputStream saveOriginalFile( InputStream inputStreamOfOrignalFile) throws IOException { ByteArrayOutputStream baos = new ByteArrayOutputStream(); byte [] buffer = new byte [1024]; int len; while ((len = inputStreamOfOrignalFile.read(buffer)) > -1) { baos.write(buffer, 0, len); } baos.flush(); return new ByteArrayInputStream(baos.toByteArray()); } /** * Replace the current file with the original one before any modifications * * @param url * @param originalFile * @param outOfCurrentFile * @ throws ConfigurationException * @since 1.9 */ private void reloadOriginalFile(URL url, ByteArrayInputStream originalFile, OutputStream outOfCurrentFile) throws ConfigurationException { if (outOfCurrentFile != null && originalFile != null ) { try { int nextChar; while ((nextChar = originalFile.read()) != -1) outOfCurrentFile.write(( char ) nextChar); outOfCurrentFile.write('\n'); outOfCurrentFile.flush(); } catch (IOException ioe) { throw new ConfigurationException( "Could not save to URL " + url, ioe); } } }
          Hide
          Laurent Michelet added a comment -

          Patch for CONFIGURATION-136 based on a 1.5 release

          Show
          Laurent Michelet added a comment - Patch for CONFIGURATION-136 based on a 1.5 release
          Hide
          Laurent Michelet added a comment -

          Patch for CONFIGURATION-136 based on a 1.8 release

          Show
          Laurent Michelet added a comment - Patch for CONFIGURATION-136 based on a 1.8 release
          Hide
          Oliver Heger added a comment -

          Thanks for your contribution, but we need a patch of the source code (that's why it's called Open Source . Then we can incorporate it in our code base for the next release. Your IDE should be able to generate the patch, otherwise you can use svn diff on the command line. More information can be found at http://commons.apache.org/patches.html.

          Show
          Oliver Heger added a comment - Thanks for your contribution, but we need a patch of the source code (that's why it's called Open Source . Then we can incorporate it in our code base for the next release. Your IDE should be able to generate the patch, otherwise you can use svn diff on the command line. More information can be found at http://commons.apache.org/patches.html .
          Hide
          Laurent Michelet added a comment -

          Maybe it's because I get used to work with proprietary source code

          Show
          Laurent Michelet added a comment - Maybe it's because I get used to work with proprietary source code
          Hide
          Oliver Heger added a comment -

          I had a look at the patch, but I am not sure whether I fully understand what you intend. I expected something similar to what Emmanuel had suggested in his comments, i.e. a modification of the reload() method to temporarily store the configuration's content before it is loaded again from disk.

          But you modified the save() method. It looks like that you try to restore the original content of the configuration if an error occurs during saving. This does not solve the problem this ticket is about.

          Also, are you sure that your solution is robust? I mean, if you get an error on saving (maybe because the disk is full or not readable), how likely is it that the rollback operation succeeds?

          Show
          Oliver Heger added a comment - I had a look at the patch, but I am not sure whether I fully understand what you intend. I expected something similar to what Emmanuel had suggested in his comments, i.e. a modification of the reload() method to temporarily store the configuration's content before it is loaded again from disk. But you modified the save() method. It looks like that you try to restore the original content of the configuration if an error occurs during saving. This does not solve the problem this ticket is about. Also, are you sure that your solution is robust? I mean, if you get an error on saving (maybe because the disk is full or not readable), how likely is it that the rollback operation succeeds?
          Hide
          Oliver Heger added a comment -

          This has been solved by the rework of the reloading mechanism which is now handled by configuration builders.

          Show
          Oliver Heger added a comment - This has been solved by the rework of the reloading mechanism which is now handled by configuration builders.

            People

            • Assignee:
              Unassigned
              Reporter:
              nicolas de loof
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development