Generated move constructors marked as noexcept, but may throw exceptions

[arthur-tacca]

As discussed in #2791, generated message classes have recently been given move constructors and move assignment operators. However, they are not marked noexcept. This means that they are not used in standard containers in situations where it would be desirable. [Edit: Sorry, they are marked noexcept, so the complaint below does not apply. However, I believe they are incorrectly marked noexcept; see the comment below.]

Consider the following example snippet:

std::vector<MyMsg> messages;
messages.push_back(makeNewMessage("a"));
messages.push_back(makeNewMessage("b"));

Thanks to the new move constructor, on line 2 it is guaranteed that the copy constructor of MyMsg will not be invoked; instead, the move constructor will be used by the push_back() method for the "a" message. Again, on line 3, the push_back() method will use the move constructor on the "b" message to add it to the vector. However, if there is not enough capacity to fit the "b" message then the vector will first reallocate it members, and at this point it will use the copy constructor on the "a" message. This is because it is impossible to perform reallocation in a way that provides the strong exception guarantee by using a move constructor that might throw an exception. Search the web for std::move_if_noexcept for more information.

In my opinion, this is an important use case of move constructors, and I think many people would be surprised to see the copy constructor invoked in the above snippet. I don't see any harm in marking these functions noexcept because there really is no way for them to throw exceptions, and they are only included when the compiler is known to be C++11 compliant.

[arthur-tacca]

I'm very sorry, I must've been looking at an old version of the library. The experimental version of move support (#ifdef PROTO_EXPERIMENTAL_ENABLE_MOVE) did not use noexcept specifiers, but the very change I linked to in the other ticket shows the noexcept specifier being added to both the move constructor. So what I was requested is already done.

However, I noticed a problem with how this feature is implemented. Even though these two functions are marked noexcept ... they may throw an exception! Look at the implementation:

Duration(Duration&& from) noexcept
  : Duration() {
  *this = ::std::move(from);
}

inline Duration& operator=(Duration&& from) noexcept {
  if (GetArenaNoVirtual() == from.GetArenaNoVirtual()) {
    if (this != &from) InternalSwap(&from);
  } else {
    CopyFrom(from);
  }
  return *this;
}

If move constructing from a message with an arena, or move assigning between objects with different arenas, then CopyFrom is called, which of course may throw an exception due to low memory (or perhaps due to the destination arena being exhausted? I don't know how protobuf arenas work). Obviously this would be bad, because it would cause an immediate program termination.

I am not sure what the solution to this is. I am a bit confused about the implementation in the first place: surely if you move construct (or indeed copy construct) from an arena-based message, then you usually want the new object to use the same arena? A move constructor implemented that way really would never throw exceptions. (The move assignment operator would still be a lost cause, but removing its noexcept would be less of a problem.) But this probably just reflects my ignorance about arenas.

[xfxyjwf]

This is partly a result of our Google C++ Style Guide where exceptions are not allowed:
https://google.github.io/styleguide/cppguide.html#Exceptions

So in Google code base, throwing an exception equals program termination. Therefore marking protobuf move constructors noexcept has no negative consequences even if they may actual throw.

I don't really have a great answer here. The movable operator= can't assume the two message are from the same arena because we do have use cases where they are from different arenas. It seems to me the CopyFrom() call is inevitable. To make code work correctly with the presence of exceptions, the only solution I can think of is to remove the noexcept annotation...

[arthur-tacca]

I suggest that for the move assignment, it is probably fine to just remove noexcept. The main thing I was worried about was message objects in containers, which only use the move constructor. Of course, perhaps I've missed some other situation where move assignment is important.

For the move constructor, I think noexcept could truthfully be left in place by changing the behaviour when the source object uses an arena. This situation is limited because it only arises if there is an explicit call to std::move (or equivalent), because arenas return pointers to objects. Moving from a message with an arena is documented to already have defined behaviour (perform a copy), but I suspect that most code using this, if it exists, is accidental. I can think of two alternatives.

Option 1: When move constructing a message, force the destination message to use the same arena as the source. This means the move can be implemented as a simple swap (see note below) even for objects with an arena. This would possibly be quite a big change, in two related respects:

• It could be a big code change in the library, because messages currently only use an arena for submessages if their own memory was allocated with that arena. It's possible that this assumption is currently used in Message, either deliberately or accidentally.
• It could be argued that it is a big API change to the library. Currently, messages returned by value or allocated on the stack cannot use arenas (for submessages). Developers seeing these variables/return values may be surprised in future if this no longer holds.

Option 2: Make it undefined behaviour to move from a message with an arena (with GOOGLE_DCHECK). Forcing an abort(), is quite drastic. But as I said, I believe anyone who encounters this is more likely to believe they were getting constant-time behaviour, in which case it is better they find out about it during testing than in production. Assuming, that is, that they find it during testing ...

These two options apply equally well to move constructors for RepeatedField and RepeatedPtrField, if those get added (also see note below).

Note about simple swap: The only worry I can think of here is making sure that the moved-from object still has a reference to the arena. If the arena is in some separately-allocated Rep object (as it is in RepeatedPtrField) then there may need to an allocation after all.

[gerben-s]

Protobuffers doesn't throw exceptions. Throwing an exception is equivalent to aborting for us, so if any part (for instance new) throws in the proto library, you should see protos as inconsistent and only aborting is defined. Given this semantics adding noexcept is correct. Furthermore move-semantics is only useful with noexcept otherwise containers will not use move, so we can't remove noexcept.

Arguably noexcept should be added to ALL proto functions so that abortion is guaranteed instead of continuing with a inconsistent proto system which can't even be cleaned up.