Author: ivan
Date: Mon Oct 17 13:49:05 2016
New Revision: 1765286

URL: http://svn.apache.org/viewvc?rev=1765286&view=rev
Log:
Implement standard lifetime semantics for svn_xml_parser_t: the object will be
automatically freed on pool cleanup. But it still can be freed explicitly
using svn_xml_free_parser(). It's the same behavior we already have for
svn_sqlite__db_t and similar.

* subversion/include/svn_xml.h
  (svn_xml_make_parser): Document existing and new behavior regarding
   svn_xml_parser_t lifetime.

* subversion/libsvn_subr/xml.c
  (parser_cleanup): New.
  (svn_xml_make_parser): Do not create SUBPOOL and allocate svn_parser_t from
   provided POOL. Register pool cleanup handler to free Expat parser.

* subversion/tests/libsvn_subr/xml-test.c
  (test_parser_free): New test.
  (test_funcs): Add test_parser_free to the test list.

Modified:
    subversion/trunk/subversion/include/svn_xml.h
    subversion/trunk/subversion/libsvn_subr/xml.c
    subversion/trunk/subversion/tests/libsvn_subr/xml-test.c

Modified: subversion/trunk/subversion/include/svn_xml.h
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_xml.h?rev=1765286&r1=1765285&r2=1765286&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_xml.h (original)
+++ subversion/trunk/subversion/include/svn_xml.h Mon Oct 17 13:49:05 2016
@@ -169,7 +169,15 @@ typedef void (*svn_xml_char_data)(void *
                                   apr_size_t len);
 
 
-/** Create a general Subversion XML parser */
+/** Create a general Subversion XML parser.
+ *
+ * The @c svn_xml_parser_t object itself will be allocated from @a pool,
+ * but some internal structures may be allocated out of pool.  Use
+ * svn_xml_free_parser() to free all memory used by the parser.
+ *
+ * Since Subversion 1.10 parser will be freed automatically on pool
+ * cleanup or by svn_xml_free_parser() call.
+ */
 svn_xml_parser_t *
 svn_xml_make_parser(void *baton,
                     svn_xml_start_elem start_handler,

Modified: subversion/trunk/subversion/libsvn_subr/xml.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/xml.c?rev=1765286&r1=1765285&r2=1765286&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/xml.c (original)
+++ subversion/trunk/subversion/libsvn_subr/xml.c Mon Oct 17 13:49:05 2016
@@ -364,6 +364,19 @@ static void expat_data_handler(void *use
 
 /*** Making a parser. ***/
 
+static apr_status_t parser_cleanup(void *data)
+{
+  svn_xml_parser_t *svn_parser = data;
+
+  /* Free Expat parser. */
+  if (svn_parser->parser)
+    {
+      XML_ParserFree(svn_parser->parser);
+      svn_parser->parser = NULL;
+    }
+  return APR_SUCCESS;
+}
+
 svn_xml_parser_t *
 svn_xml_make_parser(void *baton,
                     svn_xml_start_elem start_handler,
@@ -372,8 +385,6 @@ svn_xml_make_parser(void *baton,
                     apr_pool_t *pool)
 {
   svn_xml_parser_t *svn_parser;
-  apr_pool_t *subpool;
-
   XML_Parser parser = XML_ParserCreate(NULL);
 
   XML_SetElementHandler(parser,
@@ -382,22 +393,23 @@ svn_xml_make_parser(void *baton,
   XML_SetCharacterDataHandler(parser,
                               data_handler ? expat_data_handler : NULL);
 
-  /* ### we probably don't want this pool; or at least we should pass it
-     ### to the callbacks and clear it periodically.  */
-  subpool = svn_pool_create(pool);
-
-  svn_parser = apr_pcalloc(subpool, sizeof(*svn_parser));
+  svn_parser = apr_pcalloc(pool, sizeof(*svn_parser));
 
   svn_parser->parser = parser;
   svn_parser->start_handler = start_handler;
   svn_parser->end_handler = end_handler;
   svn_parser->data_handler = data_handler;
   svn_parser->baton = baton;
-  svn_parser->pool = subpool;
+  svn_parser->pool = pool;
 
   /* store our parser info as the UserData in the Expat parser */
   XML_SetUserData(parser, svn_parser);
 
+  /* Register pool cleanup handler to free Expat XML parser on cleanup,
+     if svn_xml_free_parser() was not called explicitly. */
+  apr_pool_cleanup_register(svn_parser->pool, svn_parser,
+                            parser_cleanup, apr_pool_cleanup_null);
+
   return svn_parser;
 }
 
@@ -406,11 +418,7 @@ svn_xml_make_parser(void *baton,
 void
 svn_xml_free_parser(svn_xml_parser_t *svn_parser)
 {
-  /* Free the expat parser */
-  XML_ParserFree(svn_parser->parser);
-
-  /* Free the subversion parser */
-  svn_pool_destroy(svn_parser->pool);
+  apr_pool_cleanup_run(svn_parser->pool, svn_parser, parser_cleanup);
 }
 
 

Modified: subversion/trunk/subversion/tests/libsvn_subr/xml-test.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/xml-test.c?rev=1765286&r1=1765285&r2=1765286&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/xml-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/xml-test.c Mon Oct 17 
13:49:05 2016
@@ -176,6 +176,42 @@ test_invalid_xml_signal_bailout(apr_pool
   return SVN_NO_ERROR;
 }
 
+static svn_error_t *
+test_parser_free(apr_pool_t *pool)
+{
+  int i;
+  apr_pool_t *iterpool;
+
+  /* Test explicit svn_xml_free_parser() calls. */
+  iterpool = svn_pool_create(pool);
+  for (i = 0; i < 100; i++)
+  {
+      svn_xml_parser_t *parser;
+
+      svn_pool_clear(iterpool);
+
+      parser = svn_xml_make_parser(&parser, NULL, NULL, NULL, iterpool);
+      svn_xml_free_parser(parser);
+  }
+  svn_pool_destroy(iterpool);
+
+  /* Test parser free using pool cleanup. */
+  iterpool = svn_pool_create(pool);
+  for (i = 0; i < 100; i++)
+  {
+      svn_xml_parser_t *parser;
+
+      svn_pool_clear(iterpool);
+
+      parser = svn_xml_make_parser(&parser, NULL, NULL, NULL, iterpool);
+      /* We didn't call svn_xml_free_parser(): the parser will be freed on
+         pool cleanup. */
+  }
+  svn_pool_destroy(iterpool);
+
+  return SVN_NO_ERROR;
+}
+
 /* The test table.  */
 static int max_threads = 1;
 
@@ -190,6 +226,8 @@ static struct svn_test_descriptor_t test
                    "test svn_xml_signal_bailout()"),
     SVN_TEST_PASS2(test_invalid_xml_signal_bailout,
                    "test svn_xml_signal_bailout() for invalid XML"),
+    SVN_TEST_PASS2(test_parser_free,
+                   "test svn_xml_parser_free()"),
     SVN_TEST_NULL
   };
 


Reply via email to