From 0b6b57f9acaa2ec648bf582ff67851331f8e6eef Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Mon, 14 Mar 2016 14:38:45 +0300 Subject: Use serializable transaction isolation level --- brep/buildfile | 9 ++- brep/database-module | 55 +++++++++++++ brep/database-module.cxx | 50 ++++++++++++ brep/database.cxx | 12 +-- brep/mod-package-details | 7 +- brep/mod-package-details.cxx | 10 +-- brep/mod-package-search | 7 +- brep/mod-package-search.cxx | 14 ++-- brep/mod-package-version-details | 7 +- brep/mod-package-version-details.cxx | 10 +-- brep/mod-repository-details | 7 +- brep/mod-repository-details.cxx | 10 +-- brep/mod-repository-root.cxx | 2 +- brep/module | 5 +- brep/module.cxx | 11 ++- brep/options.cli | 25 ++++-- etc/brep-module.conf | 20 +++-- load/load.cli | 6 +- load/load.cxx | 37 +++++---- migrate/migrate.cli | 6 +- migrate/migrate.cxx | 35 +++++---- tests/load/driver.cxx | 8 +- web/apache/log | 6 +- web/apache/request | 121 +++++++++++++++++----------- web/apache/request.cxx | 147 +++++++++++++++++++++++++++-------- web/apache/request.ixx | 43 +++++----- web/apache/service | 13 +++- web/apache/service.cxx | 42 ++++++---- web/apache/service.txx | 44 +++++++---- web/apache/stream | 92 +++++++++------------- web/module | 46 +++++++---- 31 files changed, 591 insertions(+), 316 deletions(-) create mode 100644 brep/database-module create mode 100644 brep/database-module.cxx diff --git a/brep/buildfile b/brep/buildfile index 360f82d..4caf93a 100644 --- a/brep/buildfile +++ b/brep/buildfile @@ -42,17 +42,18 @@ import libs += libstudxml%lib{studxml} gen = {hxx ixx cxx}{ options } src = \ + {hxx cxx}{ database } \ + {hxx cxx}{ database-module } \ {hxx cxx}{ diagnostics } \ - {hxx cxx}{ module } \ - {hxx }{ options-types } \ {hxx cxx}{ mod-package-details } \ {hxx cxx}{ mod-package-search } \ {hxx cxx}{ mod-package-version-details } \ - {hxx cxx}{ page } \ {hxx cxx}{ mod-repository-details } \ {hxx cxx}{ mod-repository-root } \ + {hxx cxx}{ module } \ + {hxx }{ options-types } \ + {hxx cxx}{ page } \ { cxx}{ services } \ - {hxx cxx}{ database } \ {hxx cxx}{ types-parsers } \ {hxx }{ wrapper-traits } \ ../web/{hxx cxx}{ mime-url-encoding } \ diff --git a/brep/database-module b/brep/database-module new file mode 100644 index 0000000..64d5aaf --- /dev/null +++ b/brep/database-module @@ -0,0 +1,55 @@ +// file : brep/database-module -*- C++ -*- +// copyright : Copyright (c) 2014-2016 Code Synthesis Ltd +// license : MIT; see accompanying LICENSE file + +#ifndef BREP_DATABASE_MODULE +#define BREP_DATABASE_MODULE + +#include // database + +#include +#include + +#include +#include + +namespace brep +{ + // A module that utilises the database. Specifically, it will retry the + // request in the face of recoverable database failures (deadlock, loss of + // connection, etc) up to a certain number of times. + // + class database_module: public module + { + protected: + database_module () = default; + + // Create a shallow copy (handling instance) if initialized and a deep + // copy (context exemplar) otherwise. + // + explicit + database_module (const database_module&); + + // Required to avoid getting warning from clang that + // database_module::init() hides module::init() virtual functions. This + // way all functions get to the same scope and become overloaded set. + // + using module::init; + + void + init (const options::db&); + + virtual bool + handle (request&, response&) = 0; + + protected: + size_t retry_; + shared_ptr db_; + + private: + virtual bool + handle (request&, response&, log&); + }; +} + +#endif // BREP_DATABASE_MODULE diff --git a/brep/database-module.cxx b/brep/database-module.cxx new file mode 100644 index 0000000..630fd89 --- /dev/null +++ b/brep/database-module.cxx @@ -0,0 +1,50 @@ +// file : brep/database-module.cxx -*- C++ -*- +// copyright : Copyright (c) 2014-2016 Code Synthesis Ltd +// license : MIT; see accompanying LICENSE file + +#include + +#include + +#include +#include + +namespace brep +{ + // While currently the user-defined copy constructor is not required (we + // don't need to deep copy nullptr's), it is a good idea to keep the + // placeholder ready for less trivial cases. + // + database_module:: + database_module (const database_module& r) + : module (r), + retry_ (r.retry_), + db_ (r.initialized_ ? r.db_ : nullptr) + { + } + + void database_module:: + init (const options::db& o) + { + retry_ = o.db_retry (); + db_ = shared_database (o); + } + + bool database_module:: + handle (request& rq, response& rs, log& l) + try + { + return module::handle (rq, rs, l); + } + catch (const odb::recoverable& e) + { + if (retry_-- > 0) + { + MODULE_DIAG; + l1 ([&]{trace << e.what () << "; " << retry_ + 1 << " retries left";}); + throw retry (); + } + + throw; + } +} diff --git a/brep/database.cxx b/brep/database.cxx index 17605c5..1f70881 100644 --- a/brep/database.cxx +++ b/brep/database.cxx @@ -41,11 +41,13 @@ namespace brep } shared_ptr d ( - make_shared (o.db_user (), - o.db_password (), - o.db_name (), - o.db_host (), - o.db_port ())); + make_shared ( + o.db_user (), + o.db_password (), + o.db_name (), + o.db_host (), + o.db_port (), + "options='-c default_transaction_isolation=serializable'")); databases[o] = d; return d; diff --git a/brep/mod-package-details b/brep/mod-package-details index 0a86fc2..4dc6b18 100644 --- a/brep/mod-package-details +++ b/brep/mod-package-details @@ -5,17 +5,15 @@ #ifndef BREP_MOD_PACKAGE_DETAILS #define BREP_MOD_PACKAGE_DETAILS -#include // database - #include #include -#include #include +#include namespace brep { - class package_details: public module + class package_details: public database_module { public: package_details () = default; @@ -38,7 +36,6 @@ namespace brep private: shared_ptr options_; - shared_ptr db_; }; } diff --git a/brep/mod-package-details.cxx b/brep/mod-package-details.cxx index a5c2e3e..10bd72c 100644 --- a/brep/mod-package-details.cxx +++ b/brep/mod-package-details.cxx @@ -18,7 +18,6 @@ #include #include #include -#include #include using namespace odb::core; @@ -30,9 +29,8 @@ using namespace brep::cli; // brep::package_details:: package_details (const package_details& r) - : module (r), - options_ (r.initialized_ ? r.options_ : nullptr), - db_ (r.initialized_ ? r.db_ : nullptr) + : database_module (r), + options_ (r.initialized_ ? r.options_ : nullptr) { } @@ -44,10 +42,10 @@ init (scanner& s) options_ = make_shared ( s, unknown_mode::fail, unknown_mode::fail); + database_module::init (*options_); + if (options_->root ().empty ()) options_->root (dir_path ("/")); - - db_ = shared_database (*options_); } template diff --git a/brep/mod-package-search b/brep/mod-package-search index 1730be4..bb762cd 100644 --- a/brep/mod-package-search +++ b/brep/mod-package-search @@ -5,17 +5,15 @@ #ifndef BREP_MOD_PACKAGE_SEARCH #define BREP_MOD_PACKAGE_SEARCH -#include // database - #include #include -#include #include +#include namespace brep { - class package_search: public module + class package_search: public database_module { public: package_search () = default; @@ -38,7 +36,6 @@ namespace brep private: shared_ptr options_; - shared_ptr db_; }; } diff --git a/brep/mod-package-search.cxx b/brep/mod-package-search.cxx index 77a06b6..9769783 100644 --- a/brep/mod-package-search.cxx +++ b/brep/mod-package-search.cxx @@ -21,7 +21,6 @@ #include #include #include -#include #include using namespace odb::core; @@ -33,9 +32,8 @@ using namespace brep::cli; // brep::package_search:: package_search (const package_search& r) - : module (r), - options_ (r.initialized_ ? r.options_ : nullptr), - db_ (r.initialized_ ? r.db_ : nullptr) + : database_module (r), + options_ (r.initialized_ ? r.options_ : nullptr) { } @@ -47,11 +45,11 @@ init (scanner& s) options_ = make_shared ( s, unknown_mode::fail, unknown_mode::fail); + database_module::init (*options_); + if (options_->root ().empty ()) options_->root (dir_path ("/")); - db_ = shared_database (*options_); - // Check that the database schema matches the current one. It's enough to // perform the check in just a single module implementation (and we don't // do in the dispatcher because it doesn't use the database). @@ -94,7 +92,8 @@ handle (request& rq, response& rs) try { name_value_scanner s (rq.parameters ()); - params = params::package_search (s, unknown_mode::fail, unknown_mode::fail); + params = params::package_search ( + s, unknown_mode::fail, unknown_mode::fail); } catch (const unknown_argument& e) { @@ -107,6 +106,7 @@ handle (request& rq, response& rs) ? "" : "?q=" + web::mime_url_encode (squery)); + static const string title ("Packages"); xml::serializer s (rs.content (), title); diff --git a/brep/mod-package-version-details b/brep/mod-package-version-details index cfbcf94..a463511 100644 --- a/brep/mod-package-version-details +++ b/brep/mod-package-version-details @@ -5,17 +5,15 @@ #ifndef BREP_MOD_PACKAGE_VERSION_DETAILS #define BREP_MOD_PACKAGE_VERSION_DETAILS -#include // database - #include #include -#include #include +#include namespace brep { - class package_version_details: public module + class package_version_details: public database_module { public: package_version_details () = default; @@ -41,7 +39,6 @@ namespace brep private: shared_ptr options_; - shared_ptr db_; }; } diff --git a/brep/mod-package-version-details.cxx b/brep/mod-package-version-details.cxx index 992b829..21da41f 100644 --- a/brep/mod-package-version-details.cxx +++ b/brep/mod-package-version-details.cxx @@ -18,7 +18,6 @@ #include #include #include -#include #include using namespace std; @@ -31,9 +30,8 @@ using namespace brep::cli; // brep::package_version_details:: package_version_details (const package_version_details& r) - : module (r), - options_ (r.initialized_ ? r.options_ : nullptr), - db_ (r.initialized_ ? r.db_ : nullptr) + : database_module (r), + options_ (r.initialized_ ? r.options_ : nullptr) { } @@ -45,10 +43,10 @@ init (scanner& s) options_ = make_shared ( s, unknown_mode::fail, unknown_mode::fail); + database_module::init (*options_); + if (options_->root ().empty ()) options_->root (dir_path ("/")); - - db_ = shared_database (*options_); } bool brep::package_version_details:: diff --git a/brep/mod-repository-details b/brep/mod-repository-details index 411d9e6..49ad629 100644 --- a/brep/mod-repository-details +++ b/brep/mod-repository-details @@ -5,17 +5,15 @@ #ifndef BREP_MOD_REPOSITORY_DETAILS #define BREP_MOD_REPOSITORY_DETAILS -#include // database - #include #include -#include #include +#include namespace brep { - class repository_details: public module + class repository_details: public database_module { public: repository_details () = default; @@ -38,7 +36,6 @@ namespace brep private: shared_ptr options_; - shared_ptr db_; }; } diff --git a/brep/mod-repository-details.cxx b/brep/mod-repository-details.cxx index f040be6..b0e2b95 100644 --- a/brep/mod-repository-details.cxx +++ b/brep/mod-repository-details.cxx @@ -24,7 +24,6 @@ #include #include #include -#include #include using namespace std; @@ -37,9 +36,8 @@ using namespace brep::cli; // brep::repository_details:: repository_details (const repository_details& r) - : module (r), - options_ (r.initialized_ ? r.options_ : nullptr), - db_ (r.initialized_ ? r.db_ : nullptr) + : database_module (r), + options_ (r.initialized_ ? r.options_ : nullptr) { } @@ -51,11 +49,11 @@ init (scanner& s) options_ = make_shared ( s, unknown_mode::fail, unknown_mode::fail); + database_module::init (*options_); + if (options_->root ().empty ()) options_->root (dir_path ("/")); - db_ = shared_database (*options_); - tzset (); // To use butl::to_stream() later on. } diff --git a/brep/mod-repository-root.cxx b/brep/mod-repository-root.cxx index 7c27a60..7d0adc9 100644 --- a/brep/mod-repository-root.cxx +++ b/brep/mod-repository-root.cxx @@ -40,7 +40,7 @@ namespace brep cookies () {return request_.cookies ();} virtual istream& - content () {return request_.content ();} + content (bool buffer) {return request_.content (buffer);} private: request& request_; diff --git a/brep/module b/brep/module index 52106cd..3796399 100644 --- a/brep/module +++ b/brep/module @@ -62,7 +62,7 @@ namespace brep const basic_mark info (severity::info, \ this->log_writer_, \ __PRETTY_FUNCTION__); \ - const basic_mark trace (severity::info, \ + const basic_mark trace (severity::trace, \ this->log_writer_, \ __PRETTY_FUNCTION__) @@ -76,7 +76,8 @@ namespace brep // Trace verbosity level. // // 0 - tracing disabled. - // 1 - @@ TODO: document + // 1 - brief information regarding irregular situations, which not being + // an error can be of some interest. // 2 - @@ TODO: document // // While uint8 is more than enough, use uint16 for the ease of printing. diff --git a/brep/module.cxx b/brep/module.cxx index 1257d82..40acad9 100644 --- a/brep/module.cxx +++ b/brep/module.cxx @@ -201,7 +201,9 @@ namespace brep // Read brep::module configuration. // - static option_descriptions od (convert (options::module::description ())); + static option_descriptions od ( + convert (options::module::description ())); + name_values mo (filter (opts, od)); name_value_scanner s (mo); options::module o (s, cli::unknown_mode::fail, cli::unknown_mode::fail); @@ -311,7 +313,12 @@ namespace brep // Considered using lambda for mapping but looks too verbose while can // be a bit safer in runtime. // - static int s[] = {APLOG_ERR, APLOG_WARNING, APLOG_INFO, APLOG_TRACE1}; + // Use APLOG_INFO (as opposed to APLOG_TRACE1) as a mapping for + // severity::trace. "LogLevel trace1" configuration directive switches + // on the avalanche of log messages from various modules. Would be good + // to avoid wading through them. + // + static int s[] = {APLOG_ERR, APLOG_WARNING, APLOG_INFO, APLOG_INFO}; for (const auto& e: d) { diff --git a/brep/options.cli b/brep/options.cli index f3d26d0..bcb335e 100644 --- a/brep/options.cli +++ b/brep/options.cli @@ -54,15 +54,16 @@ namespace brep string db-name = "brep" { "", - "Database name. If not specified, then '\cb{brep}' is used by default." + "Database name. If not specified, then '\cb{brep}' is used by + default." } string db-host { "", - "Database host name, address, or socket. If not specified, then connect - to \cb{localhost} using the operating system-default mechanism - (Unix-domain socket, etc)." + "Database host name, address, or socket. If not specified, then + connect to \cb{localhost} using the operating system-default + mechanism (Unix-domain socket, etc)." } uint16_t db-port = 0 @@ -70,6 +71,14 @@ namespace brep "", "Database port number. If not specified, the default port is used." } + + size_t db-retry = 10 + { + "", + "The maximum number of times to retry database transactions in the + face of recoverable failures (deadlock, loss of connection, etc). The + default is 10." + } }; class page @@ -84,10 +93,10 @@ namespace brep vector menu; { "", - "Web page menu. Each entry is displayed in the page header in the order - specified and aligned to the right edge. A link target that starts - with '\cb{/}' or contains '\cb{:}' is used as is. Otherwise, it is - prefixed with the repository web interface root." + "Web page menu. Each entry is displayed in the page header in the + order specified and aligned to the right edge. A link target that + starts with '\cb{/}' or contains '\cb{:}' is used as is. Otherwise, + it is prefixed with the repository web interface root." } }; diff --git a/etc/brep-module.conf b/etc/brep-module.conf index 8525135..05fd278 100644 --- a/etc/brep-module.conf +++ b/etc/brep-module.conf @@ -1,7 +1,7 @@ # Configuration file for the brep module (note: this is not an apache2 .conf -# file but it can be converted to one by prefixing all the options with brep-). -# See brep(1) for detailed description of each configuration option. Commented -# out options indicate their default values. +# file but it can be converted to one by prefixing all the options with +# brep-). See brep(1) for detailed description of each configuration option. +# Commented out options indicate their default values. # # Web page logo. It is displayed in the page header aligned to the left edge. @@ -10,9 +10,9 @@ # logo "" # Web page menu. Each entry is displayed in the page header in the order -# specified and aligned to the right edge. A link target that starts with '/' or -# contains ':' is used as is. Otherwise, it is prefixed with the repository web -# interface root. +# specified and aligned to the right edge. A link target that starts with '/' +# or contains ':' is used as is. Otherwise, it is prefixed with the repository +# web interface root. # menu Packages= menu About=?about @@ -50,6 +50,12 @@ menu About=?about # db-port +# The maximum number of times to retry database transactions in the +# face of recoverable failures (deadlock, loss of connection, etc). +# +# db-retry 10 + + # Trace verbosity. Disabled by default. # -# verbosity 1 +# verbosity 0 diff --git a/load/load.cli b/load/load.cli index 5c30c14..c3aeec4 100644 --- a/load/load.cli +++ b/load/load.cli @@ -96,8 +96,10 @@ class options \cb{0} Successful termination. -\cb{1} \cb{brep-load} or \l{brep-migrate(1)} instance is running. Try +\cb{1} Fatal error. + +\cb{2} \cb{brep-load} or \l{brep-migrate(1)} instance is running. Try again. -\cb{2} Fatal error. +\cb{3} The database recoverable error. Try again. " diff --git a/load/load.cxx b/load/load.cxx index 7786e0e..1a485dd 100644 --- a/load/load.cxx +++ b/load/load.cxx @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -602,7 +603,8 @@ resolve_dependencies (package& p, database& db) { auto c (*d.constraint); - if (c.min_version && c.max_version && *c.min_version == *c.max_version) + if (c.min_version && c.max_version && + *c.min_version == *c.max_version) { const version& v (*c.min_version); q = q && compare_version_eq (vm, v, v.revision != 0); @@ -755,28 +757,30 @@ try // If the pager failed, assume it has issued some diagnostics. // - return p.wait () ? 0 : 2; + return p.wait () ? 0 : 1; } if (argc < 2) { cerr << "error: configuration file path argument expected" << endl << help_info << endl; - return 2; + return 1; } if (argc > 2) { cerr << "error: unexpected argument encountered" << endl << help_info << endl; - return 2; + return 1; } - odb::pgsql::database db (ops.db_user (), - ops.db_password (), - ops.db_name (), - ops.db_host (), - ops.db_port ()); + odb::pgsql::database db ( + ops.db_user (), + ops.db_password (), + ops.db_name (), + ops.db_host (), + ops.db_port (), + "options='-c default_transaction_isolation=serializable'"); // Prevent several brep-load/migrate instances from updating DB // simultaneously. @@ -791,7 +795,7 @@ try { cerr << "error: database schema differs from the current one" << endl << " info: use brep-migrate to migrate the database" << endl; - return 2; + return 1; } // Load the description of all the internal repositories from the @@ -856,21 +860,26 @@ try catch (const database_locked&) { cerr << "brep-load or brep-migrate instance is running" << endl; - return 1; + return 2; +} +catch (const recoverable& e) +{ + cerr << "database recoverable error: " << e.what () << endl; + return 3; } catch (const cli::exception& e) { cerr << "error: " << e << endl << help_info << endl; - return 2; + return 1; } catch (const failed&) { - return 2; // Diagnostics has already been issued. + return 1; // Diagnostics has already been issued. } // Fully qualified to avoid ambiguity with odb exception. // catch (const std::exception& e) { cerr << "error: " << e.what () << endl; - return 2; + return 1; } diff --git a/migrate/migrate.cli b/migrate/migrate.cli index dcbf667..1d62504 100644 --- a/migrate/migrate.cli +++ b/migrate/migrate.cli @@ -114,8 +114,10 @@ class options \cb{0} Successful termination. -\cb{1} \cb{brep-migrate} or \l{brep-load(1)} instance is running. Try +\cb{1} Fatal error. + +\cb{2} \cb{brep-migrate} or \l{brep-load(1)} instance is running. Try again. -\cb{2} Fatal error. +\cb{3} The database recoverable error. Try again. " diff --git a/migrate/migrate.cxx b/migrate/migrate.cxx index 70ba54f..aa11ecc 100644 --- a/migrate/migrate.cxx +++ b/migrate/migrate.cxx @@ -170,8 +170,8 @@ drop (database& db) const { for (const auto& s: drop_statements_) // If the statement execution fails, the corresponding source file line - // number is not reported. The line number could be usefull for the utility - // implementer only. The errors seen by the end-user should not be + // number is not reported. The line number could be usefull for the + // utility implementer only. The errors seen by the end-user should not be // statement-specific. // db.execute (s); @@ -226,28 +226,30 @@ try // If the pager failed, assume it has issued some diagnostics. // - return p.wait () ? 0 : 2; + return p.wait () ? 0 : 1; } if (argc > 1) { cerr << "error: unexpected argument encountered" << endl << help_info << endl; - return 2; + return 1; } if (ops.recreate () && ops.drop ()) { cerr << "error: inconsistent options specified" << endl << help_info << endl; - return 2; + return 1; } - odb::pgsql::database db (ops.db_user (), - ops.db_password (), - ops.db_name (), - ops.db_host (), - ops.db_port ()); + odb::pgsql::database db ( + ops.db_user (), + ops.db_password (), + ops.db_name (), + ops.db_host (), + ops.db_port (), + "options='-c default_transaction_isolation=serializable'"); // Prevent several brep-migrate/load instances from updating DB // simultaneously. @@ -329,21 +331,26 @@ try catch (const database_locked&) { cerr << "brep-migrate or brep-load instance is running" << endl; - return 1; + return 2; +} +catch (const recoverable& e) +{ + cerr << "database recoverable error: " << e.what () << endl; + return 3; } catch (const cli::exception& e) { cerr << "error: " << e << endl << help_info << endl; - return 2; + return 1; } catch (const failed&) { - return 2; // Diagnostics has already been issued. + return 1; // Diagnostics has already been issued. } // Fully qualified to avoid ambiguity with odb exception. // catch (const std::exception& e) { cerr << "error: " << e.what () << endl; - return 2; + return 1; } diff --git a/tests/load/driver.cxx b/tests/load/driver.cxx index bb46033..c2907b6 100644 --- a/tests/load/driver.cxx +++ b/tests/load/driver.cxx @@ -84,7 +84,13 @@ main (int argc, char* argv[]) // Check persistent objects validity. // - odb::pgsql::database db ("", "", "brep", argv[3], stoul (argv[5])); + odb::pgsql::database db ( + "", + "", + "brep", + argv[3], + stoul (argv[5]), + "options='-c default_transaction_isolation=serializable'"); { session s; diff --git a/web/apache/log b/web/apache/log index 7318e32..291f8de 100644 --- a/web/apache/log +++ b/web/apache/log @@ -5,7 +5,7 @@ #ifndef WEB_APACHE_LOG #define WEB_APACHE_LOG -#include // request_rec +#include // request_rec, server_rec #include #include // module @@ -44,7 +44,7 @@ namespace web const char* msg) const noexcept { if (file && *file) - file = nullptr; // skip file/line placeholder from log line. + file = nullptr; // Skip file/line placeholder from log line. level = std::min (level, APLOG_TRACE8); @@ -59,7 +59,7 @@ namespace web func, msg); else - // skip function name placeholder from log line + // Skip function name placeholder from log line. // ap_log_error (file, line, diff --git a/web/apache/request b/web/apache/request index ab69dff..9540561 100644 --- a/web/apache/request +++ b/web/apache/request @@ -5,23 +5,14 @@ #ifndef WEB_APACHE_REQUEST #define WEB_APACHE_REQUEST -#include +#include // request_rec, HTTP_*, OK, M_POST -#include -#include -#include - -#include #include #include // unique_ptr #include -#include #include #include -#include // move() #include -#include -#include #include #include @@ -30,23 +21,63 @@ namespace web { namespace apache { + // The state of the request processing, reflecting an interaction with + // Apache API (like reading/writing content function calls), with no + // buffering taken into account. Any state different from the initial + // suppose that some irrevocable interaction with Apache API have + // happened, so request processing should be either completed, or + // reported as failed. State values are ordered in a sense that the + // higher value reflects the more advanced stage of processing, so the + // request current state value may not decrease. + // + enum class request_state + { + // Denotes the initial stage of the request handling. At this stage + // the request line and headers are already parsed by Apache. + // + initial, + + // Reading the request content. + // + reading, + + // Adding the response headers (cookies in particular). + // + headers, + + // Writing the response content. + // + writing + }; + class request: public web::request, public web::response, - public write_state + public stream_state { friend class service; - request (request_rec* rec) noexcept: rec_ (rec) {rec_->status = HTTP_OK;} + request (request_rec* rec) noexcept + : rec_ (rec) + { + rec_->status = HTTP_OK; + } + + request_state + state () const noexcept {return state_;} - // Flush of buffered content. + // Flush the buffered response content if present. The returned value + // should be passed to Apache API on request handler exit. // int flush (); - // Return true if content have been sent to the client, false otherwise. + // Prepare for the request re-processing if possible (no unbuffered + // read/write operations have been done). Throw sequence_error + // otherwise. In particular, the preparation can include the response + // content buffer cleanup, the request content buffer rewind. // - bool - get_write_state () const noexcept {return write_state_;} + void + rewind (); // Get request path. // @@ -56,7 +87,7 @@ namespace web // Get request body data stream. // virtual std::istream& - content (); + content (bool buffer = false); // Get request parameters. // @@ -70,7 +101,8 @@ namespace web // Get response status code. // - status_code status () const noexcept {return rec_->status;} + status_code + status () const noexcept {return rec_->status;} // Set response status code. // @@ -89,10 +121,11 @@ namespace web virtual void cookie (const char* name, const char* value, - const std::chrono::seconds* max_age = 0, - const char* path = 0, - const char* domain = 0, - bool secure = false); + const std::chrono::seconds* max_age = nullptr, + const char* path = nullptr, + const char* domain = nullptr, + bool secure = false, + bool buffer = true); private: // Get application/x-www-form-urlencoded form data. @@ -103,37 +136,35 @@ namespace web void parse_parameters (const char* args); + // Advance the request processing state. Noop if new state is equal to + // the current one. Throw sequence_error if the new state is less then + // the current one. Can throw invalid_request if HTTP request is + // malformed. + // + void + state (request_state); + + // stream_state members implementation. + // virtual void - set_write_state () - { - if (!write_state_) - { - // Preparing to write a response read and discard request - // body if any. - // - int r (ap_discard_request_body (rec_)); - - if (r != OK) - { - throw invalid_request (r); - } - - write_state_ = true; - } - } + set_read_state () {state (request_state::reading);} + + virtual void + set_write_state () {state (request_state::writing);} private: request_rec* rec_; - bool buffer_ {true}; - bool write_state_ {false}; - std::unique_ptr out_buf_; - std::unique_ptr out_; - std::unique_ptr in_buf_; - std::unique_ptr in_; + request_state state_ = request_state::initial; + path_type path_; std::unique_ptr parameters_; std::unique_ptr cookies_; std::unique_ptr form_data_; + std::unique_ptr in_buf_; + std::unique_ptr in_; + + std::unique_ptr out_buf_; + std::unique_ptr out_; }; } } diff --git a/web/apache/request.cxx b/web/apache/request.cxx index 2e03190..b0b4d61 100644 --- a/web/apache/request.cxx +++ b/web/apache/request.cxx @@ -4,22 +4,30 @@ #include -#include +#include // apr_table_*, apr_array_header_t +#include // apr_pstrdup() + +#include // request_rec, HTTP_*, OK +#include // ap_*() #include // strcasecmp() -#include -#include +#include // strftime(), time_t #include #include // unique_ptr +#include +#include #include #include #include -#include +#include // str*(), size_t #include // move() -#include +#include // invalid_argument +#include // current_exception() #include +#include + #include using namespace std; @@ -28,16 +36,92 @@ namespace web { namespace apache { + void request:: + state (request_state s) + { + assert (s != request_state::initial); + + if (s == state_) + return; // Noop. + + if (s < state_) + { + // Can't "unwind" irrevocable interaction with Apache API. + // + static const char* names[] = { + "initial", "reading", "headers", "writing"}; + + string str ("web::apache::request::set_state: "); + str += names[static_cast (state_)]; + str += " to "; + str += names[static_cast (s)]; + + throw sequence_error (move (str)); + } + + if (s == request_state::reading) + { + // Prepare request content for reading. + // + int r (ap_setup_client_block (rec_, REQUEST_CHUNKED_DECHUNK)); + + if (r != OK) + throw invalid_request (r); + } + else if (s > request_state::reading && state_ <= request_state::reading) + { + // Read request content if any, discard whatever is received. + // + int r (ap_discard_request_body (rec_)); + + if (r != OK) + throw invalid_request (r); + } + + state_ = s; + } + + void request:: + rewind () + { + // @@ Request content buffering, and response cookies buffering are not + // supported yet. When done will be possible to rewind in broader + // range of cases. + // + + if (state_ == request_state::initial || + + // Form data have been read. Lucky case, can rewind. + // + (state_ == request_state::reading && + dynamic_cast (in_buf_.get ()) != nullptr)) + { + out_.reset (); + out_buf_.reset (); + + rec_->status = HTTP_OK; + + ap_set_content_type (rec_, nullptr); + + if (in_) + in_->seekg (0); + } + else + throw sequence_error ("web::apache::request::rewind"); + } + istream& request:: - content () + content (bool buffer) { + assert (!buffer); // Request content buffering is not implemented yet. + if (!in_) { unique_ptr in_buf (new istreambuf (rec_, *this)); in_.reset (new istream (in_buf.get ())); in_buf_ = move (in_buf); - in_->exceptions (ios::failbit | ios::badbit); + in_->exceptions (istream::failbit | istream::badbit); // Save form data now otherwise will not be available to do later // when data already read from stream. @@ -135,17 +219,29 @@ namespace web ostream& request:: content (status_code status, const string& type, bool buffer) { - if (out_ && status == rec_->status && buffer == buffer_ && + if (out_ && + + // Same status code. + // + status == rec_->status && + + // Same buffering flag. + // + buffer == + (dynamic_cast (out_buf_.get ()) != nullptr) && + + // Same content type. + // strcasecmp (rec_->content_type ? rec_->content_type : "", type.c_str ()) == 0) { + // No change, return the existing stream. + // return *out_; } - if (get_write_state ()) - { - throw sequence_error ("::web::apache::request::content"); - } + if (state_ >= request_state::writing) + throw sequence_error ("web::apache::request::content"); if (!buffer) // Request body will be discarded prior first byte of content is @@ -161,9 +257,8 @@ namespace web out_.reset (new ostream (out_buf.get ())); out_buf_ = move (out_buf); - out_->exceptions (ios::eofbit | ios::failbit | ios::badbit); + out_->exceptions (ostream::eofbit | ostream::failbit | ostream::badbit); - buffer_ = buffer; rec_->status = status; ap_set_content_type ( @@ -182,13 +277,10 @@ namespace web // where no sense to throw but still need to signal apache a // proper status code. // - if (get_write_state () && !current_exception ()) - { - throw sequence_error ("::web::apache::request::status"); - } + if (state_ >= request_state::writing && !current_exception ()) + throw sequence_error ("web::apache::request::status"); rec_->status = status; - buffer_ = true; out_.reset (); out_buf_.reset (); ap_set_content_type (rec_, nullptr); @@ -201,14 +293,10 @@ namespace web const chrono::seconds* max_age, const char* path, const char* domain, - bool secure) + bool secure, + bool buffer) { - if (get_write_state ()) - { - // Too late to send cookie if content is already written. - // - throw sequence_error ("::web::apache::request::cookie"); - } + assert (!buffer); // Cookie buffering is not implemented yet. ostringstream s; mime_url_encode (name, s); @@ -230,20 +318,15 @@ namespace web } if (path) - { s << ";Path=" << path; - } if (domain) - { s << ";Domain=" << domain; - } if (secure) - { s << ";Secure"; - } + state (request_state::headers); apr_table_add (rec_->err_headers_out, "Set-Cookie", s.str ().c_str ()); } diff --git a/web/apache/request.ixx b/web/apache/request.ixx index 8a3b32b..821aaba 100644 --- a/web/apache/request.ixx +++ b/web/apache/request.ixx @@ -4,10 +4,12 @@ #include // strncasecmp() -#include +#include // apr_table_* + +#include // ap_*() + #include -#include -#include +#include // move() namespace web { @@ -16,35 +18,34 @@ namespace web inline int request:: flush () { - if (buffer_ && out_buf_) + if (std::stringbuf* b = dynamic_cast (out_buf_.get ())) { - auto b (dynamic_cast (out_buf_.get ())); - assert (b); - + // Response content is buffered. + // std::string s (b->str ()); if (!s.empty ()) { - // Before writing response read and discard request body if any. - // - int r (ap_discard_request_body (rec_)); - - if (r == OK) + try { - set_write_state (); + state (request_state::writing); if (ap_rwrite (s.c_str (), s.length (), rec_) < 0) rec_->status = HTTP_REQUEST_TIME_OUT; } - else - rec_->status = r; + catch (const invalid_request& e) + { + rec_->status = e.status; + } } out_.reset (); out_buf_.reset (); } - return rec_->status == HTTP_OK || get_write_state () ? OK : rec_->status; + return rec_->status == HTTP_OK || state_ >= request_state::writing + ? OK + : rec_->status; } inline const std::string& request:: @@ -64,19 +65,21 @@ namespace web std::istream& istr (content ()); // Do not throw when eofbit is set (end of stream reached), and - // when failbit is set (getline() failed to extract any character). + // when failbit is set (getline() failed to extract any + // character). // - istr.exceptions (std::ios::badbit); + istr.exceptions (std::istream::badbit); std::getline (istr, *form_data_); - // Make this data the content of the input stream. + // Make this data the content of the input stream, so it's + // available for the application as well. // std::unique_ptr in_buf ( new std::stringbuf (*form_data_)); in_.reset (new std::istream (in_buf.get ())); in_buf_ = std::move (in_buf); - in_->exceptions (std::ios::failbit | std::ios::badbit); + in_->exceptions (std::istream::failbit | std::istream::badbit); } } } diff --git a/web/apache/service b/web/apache/service index 4c0d395..165ff90 100644 --- a/web/apache/service +++ b/web/apache/service @@ -5,8 +5,11 @@ #ifndef WEB_APACHE_SERVICE #define WEB_APACHE_SERVICE -#include -#include // module, ap_hook_*() +#include // apr_pool_t +#include // APR_HOOK_* + +#include // request_rec, server_rec, HTTP_*, DECLINED +#include // module, cmd_parms, ap_hook_*() #include #include // unique_ptr @@ -130,7 +133,8 @@ namespace web // The worker_initializer() function is called right after Apache // worker process is started. Called for every new process spawned. // - ap_hook_child_init (&worker_initializer, NULL, NULL, APR_HOOK_LAST); + ap_hook_child_init ( + &worker_initializer, NULL, NULL, APR_HOOK_LAST); // The request_handler () function is called for each client request. // @@ -254,7 +258,8 @@ namespace web parse_option (cmd_parms* parms, void* conf, const char* args) noexcept; const char* - add_option (context_id id, const char* name, optional value); + add_option ( + context_id id, const char* name, optional value); void finalize_config (server_rec*); diff --git a/web/apache/service.cxx b/web/apache/service.cxx index 2ca8cf0..da44530 100644 --- a/web/apache/service.cxx +++ b/web/apache/service.cxx @@ -4,10 +4,10 @@ #include -#include +#include // apr_palloc() -#include -#include +#include // server_rec +#include // command_rec, cmd_*, ap_get_module_config() #include // unique_ptr #include @@ -16,7 +16,10 @@ #include // strlen() #include +#include + #include +#include using namespace std; @@ -29,12 +32,12 @@ namespace web { assert (cmds == nullptr); - // Fill apache module directive definitions. Directives share - // common name space in apache configuration file, so to prevent name - // clash have to form directive name as a combination of module and - // option names: -