#151 Improve TRegKey error handling

[Chris Kohlert]

Currently, the TRegKey constructors may fail and leave the encapsulated HKEY handle invalid. While you can check the validity of the TRegKey object with the conversion operator THandle, there is currently no way to get the error code for the failure. The enclosed patch adds a GetError member function to TRegKey, to allow access to the error code.

However, this is only a minimal and unsatisfactory solution. Ideally, the TRegKey constructors should throw TXRegistry on failure, in accordance with good C++ coding practice. Except for the TRegConfigFile constructor, all of the code in OWLNext that uses TRegKey already assumes that successful construction of TRegKey will leave the object in a valid state (non-null handle), without doing any error checking (although, some of the client code performs error-checking on subsequent operations on the key).

PS. The patch also adds GetHandle to expose the OS handle of TRegKey. Although the class already has a THandle conversion operator for this purpose, adding GetHandle is consistent with the rest of OWLNext. The patch now excludes setting output parameters to zero in error cases, as discussed in the forum [discussion:39ed1d9276].

[Vidar Hasfjord]

Good work, Chris!

I think error handling is a big enough feature to deserve its own ticket. I have changed the ticket title and description accordingly. Consider creating a separate ticket for the GetHandle function (or since this is minor, and the class already has this capability in the form of a conversion operator, we can just add it in a separate revision without a ticket).

I've reviewed your patch, and here are some comments and nit-picks:

Why not throw exceptions instead of adding GetError? Unless there is a good reason, we should consider exceptions here. Good C++ practice establishes the class invariant in the constructor and throws an exception if it cannot be established. Adding an error state to the class invariant complicates the possible state space. Of course, adding descriptive exceptions is a big change, possibly requiring client code updates, but this ticket will target version 7, which is aiming for modernisation, so that is not much of an issue.

Note that we already have TXRegistry, which can be extended with the error code.

• Member fields names should be in capitalised camel-case, i.e. LastRegError rather than lastregerror. See our Coding Standards.
• Tickets should use our conventional labels. See Submitting bugs and feature requests. I've added "API" and "Exceptions" for this ticket.
• I've linked this ticket to the corresponding discussion thread in our forum.

[Vidar Hasfjord]

I've now added TRegKey::GetHandle [r4784] and merged this change into Owlet [r4785], so this ticket can now focus on the error handling issue.

[Vidar Hasfjord]

I've looked at the original OWL 5 source code, and it seems exception handling was considered but disabled, or never completed, for some reason. The following commented-out line of code is found in the OWL 5 version of one of the TRegKey constructors, and this line is still present in the latest code on the trunk, now refactored into TRegKey::Init (THandle, REGSAM, TCreateOK):

:::C++
TRegKey::Init(THandle baseKey, ...)
{
  ...
//  TXRegistry::Check(long(Key), Name);
}

Note that the TRegValue (const TRegValueIterator&) constructor does throw TXRegistry on failure, but this is a change since OWL 5:

:::C++
TRegValue::TRegValue(const TRegValueIterator& iter)
: Key(...), ...
{
  ...
  long r = Key.EnumValue(...);
  if (r != ERROR_SUCCESS) throw TXRegistry(...);
  ...
}

Other TRegValue constructors do not throw exceptions, though, which is inconsistent. They call TRegValue::QueryTypeAndSize, but completely ignore the error return code. The QueryTypeAndSize function is new since OWL 5, due to refactorisation, but the original code in the constructors also just ignores error handling.

:::C++
TRegValue::TRegValue(const TRegKey& key, LPCTSTR name)
: Key(key), ...
{
  QueryTypeAndSize();
}

The original OWL 5 code only uses TXRegistry in TRegSymbolTable::UpdateParams and StreamOut, as well as in TRegistry::Update, and these cases are still present in the latest code on the trunk.

Overall, in summary, the error handling in the registry classes is incomplete and inconsistent.

[Chris Kohlert]

I was under the impression that it is never a good idea in c++ to throw exceptions in constructors...

A common use pattern of the windows registry is to try to get a value out of the registry and have a default value ready if it doesnt exist, so that might be the reason why exceptions didnt make it too much in that class.
A better API would be maybe something like QueryRegistryValuePathOrDefault ... or something like it.
Too many fine grained exception handlers make the code not very readable.

[Vidar Hasfjord]

Hi Chris,

I was under the impression that it is never a good idea in c++ to throw exceptions in constructors

That is incorrect. You are probably confusing the advise about destructors, which are not allowed to throw exceptions — an unhandled exception in a destructor leads to termination of the program. See Item 51, "Destructors, deallocation and swap never fail" in "C++ Coding Standards" by Sutter and Alexandrescu.

The advise for constructors is the opposite — you should "prefer to use exceptions to report errors" (Item 72 in "C++ Coding Standards"):

Exception handling is better than the alternatives for reporting errors from constructors and operators: [...] For constructors, using error codes is not feasible because the C++ language tightly binds together constructor exceptions and constructor failures so that the two have to be synonymous; if instead we used an errno-like approach [...] then not only is the result ugly and error-prone, but it leads to misbegotten objects that don't really satisfy their type's invariant...

For example, if TRegKey construction fails, we don't really have a valid registry key object.

A better API would be maybe something like QueryRegistryValuePathOrDefault [...] Too many fine grained exception handlers make the code not very readable.

I fully agree. Expected scenarios should not be coded with exception handling. The parameter TCreateOK to the TRegKey constructor allows you to create the key if it does not exist. But if NoCreate is passed the constructor should ideally throw an exception if the key does not exist. To avoid awkward exception handling, we probably need a static Exists function (similar to std::filesystem::exists):

:::C++
if (TRegKey::Exists(parent, keyName))
{
  auto key = TRegKey{parent, keyName, samDesired, NoCreate};
  ...
}

For registry values, a convenient API like you suggest would make sense. You can create a function for this purpose quite easily:

:::C++
const auto queryRegistryValueOrDefault = [&key](tstring valName, tstring defValue) -> tstring
{
  if (key.QueryValue(valName, nullptr, nullptr, nullptr) == ERROR_SUCCESS)
  {
    const auto v = TRegValue{key, valName};
    return static_cast<LPCTSTR>(v);
  }
  else
    return defValue;
};

As for TRegKey, the check for existence of a value could be wrapped up in a static function TRegValue::Exists, or perhaps TRegKey::HasValue.

[Chris Kohlert]

Yes, guess i confused it with destructors then, thanks. and modern c++ is awesome :)

[Vidar Hasfjord]

Ok, if there is no reason to the contrary, I recommend using exceptions to improve error handling in TRegKey (and TRegValue), and I hence vote against applying your patch in its current form. I'll leave it to you to take this any further, but I'll review any code changes, if you decide to go forward.

[Vidar Hasfjord]

As mentioned, to avoid error codes and exception handling for simple registry queries in ordinary client code, we need a way to check whether a key or value exists before we try to create a TRegKey or TRegValue to represent it, respectively. As suggested, this can be implemented as static member functions in TRegKey and TRegValue:

:::C++
auto TRegKey::Exists(HKEY baseKey, const tstring& keyName) -> bool
{
  auto key = HKEY{};
  const auto r = ::RegOpenKeyEx(baseKey, keyName.c_str(), 0, KEY_READ, &key);
  if (key)
    ::RegCloseKey(key);
  return r != ERROR_FILE_NOT_FOUND;
}

auto TRegValue::Exists(const TRegKey& key, const tstring& valName) -> bool
{
  return key.QueryValue(valName, nullptr, nullptr, nullptr) != ERROR_FILE_NOT_FOUND;
}

As a further convenience, we could add another couple of static functions to optionally create a TRegKey or TRegValue object based on whether the underlying registry key or value exists, respectively. These functions can be simply implemented using the Exists functions above and std::optional:

:::C++
auto TRegKey::Query(HKEY baseKey, const tstring& keyName, REGSAM samDesired = KEY_READ) -> std::optional<TRegKey>
{
  return TRegKey::Exists(baseKey, keyName) ?
    std::make_optional<TRegKey>(baseKey, keyName, samDesired, TRegKey::NoCreate) :
    std::nullopt;
}

auto TRegValue::Query(const TRegKey& key, const tstring& valName) -> std::optional<TRegValue>
{
  return TRegValue::Exists(key, valName) ?
    std::make_optional<TRegValue>(key, valName) :
    std::nullopt;
};

This would allow us to write simple client code like the following:

:::C++
if (const auto key = TRegKey::Query(HKEY_CURRENT_USER, "Software\\TortoiseMerge"))
{
  const auto getString = [&](tstring valName, tstring defValue) -> tstring
  {
    const auto v = TRegValue::Query(*key, valName);
    return v ? static_cast<LPCTSTR>(*v) : defValue;
  };
  const auto f = getString("FontName", "DefaultFont");
  const auto n = getString("Nonsense", "DefaultNonsense");
  ...
}

Alternatively, with some refactoring of the code, TRegValue::Exists and TRegValue::Query can be implemented as non-static member functions TRegKey::HasValue and TRegKey::GetValue, respectively. I think we just need to define TRegValue before TRegKey, using a forward declaration for TRegKey.

And TRegKey::Exists and TRegKey::Query can alternatively be implemented as non-static member functions HasSubkey and GetSubkey, respectively, operating on the parent key.

I think I would prefer this alternative, as it nicely complements the existing non-static member functions GetSubkeyCount and GetValueCount.

For example, with this alternative, the client code above would be rewritten like this:

:::C++
if (const auto key = TRegKey::CurrentUser().GetSubkey("Software\\TortoiseMerge")
{
  const auto getString = [&](tstring valName, tstring defValue) -> tstring
  {
    const auto v = key->GetValue(valName);
    return v ? static_cast<LPCTSTR>(*v) : defValue;
  };
  const auto f = getString("FontName", "DefaultFont");
  const auto n = getString("Nonsense", "DefaultNonsense");
  ...
}

What do you think?

[Ognyan Chernokozhev]

It seems to me that the implementation of a static TRegKey::Query in the manner of calling first Exists() and then creating the actual registry key has a redundant sequence of first opening the key (to check if it exists), then closing it, and then opening it again.

I think that checking if a registry key exists, but not actually using it, is a rare case.
So it will be better if we have a static method that directly open a key and returns it if exists, and then this method can be used to check for key existence as well.

Having an instance method of GetSubKey is also good, and in fact may be better, since in all cases here we have an existing parent instance (at the very least one of the static root keys).

So, my vote is on an instance method that returns a TRegKey if the key exists or null otherwise.

[Vidar Hasfjord]

I have now made an experimental implementation of my suggested extensions in Owlet [r4797].

A better API would be maybe something like QueryRegistryValuePathOrDefault

My experimental implementation includes the template function GetValueOrDefault:

:::C++
template <typename T>
auto TRegKey::GetValueOrDefault(const tstring& valueName, T defaultValue) const
{
  static_assert(!std::is_pointer_v<T>, "Return type can not be pointer.");
  const auto v = GetValue(valueName);
  return v ? static_cast<T>(*v) : defaultValue;
}

This function will look up the value, and if it exists, it will try to perform a conversion on the TRegValue to type T. The type of the default value provided must be convertible to T. Note that pointers are not allowed, i.e. T cannot be LPCTSTR, because that would return a pointer to a dead object (the TRegValue object is local to the GetValueOrDefault function). But you can use owl::tstring instead.

For numeric values, you need to be specific about the data type. Only owl::uint32 and owl::uint64 are supported.

For example:

:::C++
if (const auto key = TRegKey::CurrentUser().GetSubKey("Software\\TortoiseMerge"))
{
  // String values must use standard strings.
  //
  //const auto font1 = key->GetValueOrDefault("FontName", "DefaultFont"); // Compile error: Return type can not be pointer.
  const auto font1 = key->GetValueOrDefault("FontName", tstring{"DefaultFont"}); // OK

  // We can use the std::string literal suffix to simplify.
  //
  using namespace std::literals::string_literals;
  const auto font2 = key->GetValueOrDefault("FontName", "DefaultFont"s); // OK

  // Or we can use an explicit template argument.
  //
  const auto font3 = key->GetValueOrDefault<tstring>("FontName", "DefaultFont"); // OK

  // Likewise, we must be specific about the type of numeric values.
  //
  //const auto fontSize1 = key->GetValueOrDefault("FontSize", 0); // Compile error: Ambigious user-defined conversion.
  //const auto fontSize1 = key->GetValueOrDefault("FontSize", 0ull); // TXRegistry: incompatible registry value type conversion (uint64).
  const auto fontSize1 = key->GetValueOrDefault("FontSize", uint32{0}); // OK
  const auto fontSize2 = key->GetValueOrDefault("FontSize", 0ul); // OK
  const auto fontSize3 = key->GetValueOrDefault<uint32>("FontSize", 0); // OK

  ...
}

[Chris Kohlert]

I like that, seems like a good solution.

[Vidar Hasfjord]

Jogy wrote:

It seems to me that the implementation of a static TRegKey::Query in the manner of calling first Exists() and then creating the actual registry key has a redundant sequence of first opening the key (to check if it exists), then closing it, and then opening it again.

True. In my post, that you responded to, TRegKey::Query was implemented like that, for simple exposition. But like you say, it can be optimised by combining the steps and hence avoid opening the key twice. Actually, that is how I implemented the alternative TRegKey::GetSubkey, as you can see in [r4797]:

:::C++
auto TRegKey::GetSubkey(const tstring& keyName, REGSAM samDesired) const -> optional<TRegKey>
{
  OWL_PRECONDITION(Key);
  auto subkey = HKEY{};
  const auto r = ::RegOpenKeyEx(Key, keyName.c_str(), 0, samDesired, &subkey);
  if (r != ERROR_SUCCESS && r != ERROR_FILE_NOT_FOUND) TXRegistry::Check(r, keyName.c_str());
  return subkey ? make_optional<TRegKey>(subkey, true, keyName) : nullopt; // TRegKey takes ownership.
}

In effect, this implementation filters out ERROR_SUCCESS and ERROR_FILE_NOT_FOUND as valid program states (not errors) and throws an exception for all other error values.

Chris wrote:

I like that, seems like a good solution.

OK, since we have agreement on the solution, I will merge my experimental implementation to the trunk.

Now that we will use TXRegistry more extensively, there is another issue that needs urgent fixing. The TXRegistry class holds a possibly dangling pointer to the key name. There is already a code comment about this (made by me in [r1122] as part of our work on standard string support):

:::C++
//
/// \class TXRegistry
// ~~~~~ ~~~~~~~~~~
/// TXRegistry is the object thrown when exceptions are encountered within the
/// WinSys Registry classes.
///
/// TODO: Make the Key member hold a copy of the key name rather than a weak pointer, and add string-class support.
//
class _OWLCLASS TXRegistry : public TXBase {
  public:
    TXRegistry(const tstring& msg, LPCTSTR key);
    TXRegistry(const TXRegistry& copy);

    static void Check(long stat, LPCTSTR key);

    const tchar* Key;
};

I will follow up on that TODO comment.

[Vidar Hasfjord]

The more you start looking at something, the more issues you find: I've now created a ticket regarding an issue with the TRegistry::Update and Validate functions.

See [bugs:#456].

[Vidar Hasfjord]

I have now fixed the dangling pointer issue with TXRegistry. See ticket [bugs:#457].

This means that the OWLNext source code is now ready to use TXRegistry more consistently and extensively, if you want to go forward with this ticket.

As discussed earlier, my obvious and foremost suggestion is to make the TRegKey constructors throw TXRegistry if they cannot establish a valid handle. Some client code may need to be adapted to work correctly after this change, and even some code that continues to work correctly, should probably be updated, if coded for the old behaviour.

For example, the TRegConfigFile constructor does this to check the validity of the passed HKEY:

:::C++
TRegConfigFile::TRegConfigFile(const tstring& name, HKEY root)
:
  TConfigFile(_T("")),hRoot(root)
{
  FileName = _T("Software\\");
  FileName += name;
  TRegKey testKey(hRoot);
  if(testKey==HKEY(0)){
    hRoot = 0;
    throw TXRegistry{_T("TRegConfigFile::TRegConfigFile: Root key is invalid"), testKey};
  }
}

If the TRegKey constructor is made to throw, the whole if-statement above becomes redundant. The obvious improvement to TRegConfigFile is to hold the passed root handle as a member of type TRegKey, not a HKEY like now. Then the initialisation of the member will throw an exception if the handle is invalid.

:::C++
TRegConfigFile::TRegConfigFile(const tstring& name, HKEY root)
  : TConfigFile{_T("Software\\" + name}, Root{root, false}
{}

Beyond the TRegKey constructors, you may consider whether to implement exceptions-based error handling in the member functions that currently are just thin wrappers around the corresponding Windows API functions. I suspect that a lot of client code just ignores the error code returned by these functions.

By freeing up the return value of these functions, you may be able to simplify the API. For example TRegKey::QueryInfo takes a boat-load of output pointers as arguments:

:::C++
long QueryInfo(LPTSTR class_, DWORD* classSize,
  DWORD* subkeyCount, DWORD* maxSubkeySize,
  DWORD* maxClassSize, DWORD* valueCount,
  DWORD* maxValueName, DWORD* maxValueData,
  DWORD* secDescSize, PFILETIME lastWriteTime);

With exceptions, it can be changed to return an info structure, for example:

:::C++
struct TInfo {...};
auto QueryInfo() -> TInfo;

// To be used like this:

const auto t = TRegKey{...}.QueryInfo().LastWriteTime;

PS. This member function should be const, by the way. (Changed in [r4825].)

Edit: As suggested, a new functional-style overload of TRegKey::QueryInfo has been added, and it works just as described. On errors, it throws TXRegistry. See [r4854].

[Vidar Hasfjord]

I've made some refactorisations in the OWLNext core, using the new functions added to TRegKey, leading to much simpler code in many cases. See [r4842].

[Vidar Hasfjord]

OK, as I said earlier, with the TXRegistry fix in place, OWLNext is now ready to use TXRegistry more consistently and extensively.

What would this look like? What changes am I talking about? For example, let's take a look at TRegConfigFile::WriteString. It now looks like this, after my latest refactorisation:

:::C++
bool TRegConfigFile::WriteString(LPCTSTR section, LPCTSTR entry, LPCTSTR value)
{
  PRECONDITION(section && entry && value);
  const auto k = TRegKey{hRoot, FileName + szSep + section}; CHECK(k.GetHandle());
  return k.SetValue(entry, value) == ERROR_SUCCESS;
}

With a transition to TXRegistry we can simplify the code further by using TRegValue's assignment operator:

:::C++
void TRegConfigFile::WriteString(LPCTSTR section, LPCTSTR entry, LPCTSTR value)
{
  PRECONDITION(section && entry && value);
  const auto k = TRegKey{hRoot, FileName + szSep + section};
  TRegValue{k, entry} = value;
}

Here TRegValue's assignment operator will throw TXRegistry on failure. This has already been the case for a long time. The TRegValue constructor should throw an exception if the given key is invalid, but doesn't yet. Likewise, the TRegKey constructor should throw an exception on failure to establish a valid handle, but doesn't yet.

However, note that TRegValue is designed as a lazy reference class, so the constructors should not throw an exception if the value entry in the registry is nonexistent. The check on the existence of the value should happen in the accessor functions (e.g. GetData), and exceptions on failure to create a new value or write an existing one should happen in the mutator functions (e.g. Set).

As such, TRegValue does not represent an entry in the registry. It only represents the value of a potential entry. This is unlike TRegKey which actually does represent a key in the registry (by encapsulating a valid handle to it).

Also note that we could (and ideally should) change the return value to void here, since we no longer use an error return code. This would force clients to update their error handling, if any. However, in this case, WriteString is a virtual function, so changing its signature has ramification throughout the class hierarchy. In a class hierarchy with base classes and virtual functions like here, ideally, the base class should specify in the documentation what exceptions should be expected on failure for each of its virtual functions, and then the derived classes should be written to satisfy that contract (which in our case here, may mean translation of the TXRegistry exception to the exception specified by the base class TConfigFile). Of course, without any specified contract, any C++ function can theoretically fail with any exception, unless the function signature has noexcept specified (or throw() in legacy code).

[Vidar Hasfjord]

Hi Chris, based on my latest comments and the lack of objections, I have taken ownership of the ticket. I will implement my primary suggestion, i.e. make TRegKey constructors throw.

Edit: Throwing constructors have been implemented in [r4865], and the changes have been merged into Owlet [r4866].

Please review and give me feedback on any problems you may see.

[Vidar Hasfjord]

I just updated OWLMaker to work with the latest changes in TRegKey [r4873].

This change allowed me to replace old ugly code with much cleaner code.

For example, this is the old code to locate the installation folder for Embarcadero C++Builder/RAD Studio products:

:::C++
auto TToolsetBDS2009::FindBCBLocation(const tstring& regkeypath) -> tstring
{
  const auto fullregkeypath = SoftwareKey + "\\" + regkeypath;
  const auto key = TRegKey{HKEY_LOCAL_MACHINE, fullregkeypath, KEY_READ, TRegKey::NoCreate};
  if (!key) return {};
  auto type = static_cast<uint32>(REG_SZ);
  auto size = static_cast<uint32>(_MAX_PATH);
  char buf[_MAX_PATH] = {};
  const auto r = key.QueryValue("RootDir", &type, reinterpret_cast<uint8*>(buf), &size);
  if (r != ERROR_SUCCESS) return {};
  const auto p = path{buf};
  if (!exists(p)) return {};
  return to_tstring(canonical(p));
}

And here it is now:

:::C++
auto TToolsetBDS2009::FindBCBLocation(const tstring& regkeypath) -> tstring
{
  if (const auto k = TRegKey::GetLocalMachine().GetSubkey(SoftwareKey + "\\" + regkeypath))
    if (const auto v = k->GetValue("RootDir"))
      if (const auto p = path{static_cast<LPCTSTR>(*v)}; exists(p))
        return to_tstring(canonical(p));
  return {};
}

PS. I've uploaded an updated version of OWLMaker. This also fixes a problem with the automatic update feature (see [r4876]), so you may have to manually download this update.