[c++] ClientAsyncResponseReader placement operator delete does not match placement operator new

[chwarr]

Should this be an issue in the gRPC issue tracker?

Yes, I think.

What version of gRPC and what language are you using?

C++ as of tag v1.3.4 (but similar code is present in master as of 19fd924).

What operating system (Linux, Windows, …) and version?

Windows 10 Version 1703 (OS Build 15063.296)

What runtime / compiler are you using (e.g. python version or version of gcc)

Visual C++ 2015 (19.00.24215.1 for x64) built via CMake with the 'Visual Studio 14 2015 Win64' generator.

What did you do?

Attempted to compile some code that uses grpc::ClientAsyncResponseReader.

A minimal repo can be found at https://github.com/chwarr/op-del-example.

What did you expect to see?

No build warnings.

What did you see instead?

Build warning C4291 about mismatched placement new and delete operators.

C:\src\op-del-example\grpc\include\grpc++/impl/codegen/async_unary_call.h(102): warning C4291: 'void *grpc::ClientAsyncResponseReader<int>::operator new(std::size_t,void *)': no matching operator delete found; memory will not be freed if initialization throws an exception [C:\src\build-ode\op-del-example\op-del-example.vcxproj]
C:\src\op-del-example\grpc\include\grpc++/impl/codegen/async_unary_call.h(157): note: see declaration of 'grpc::ClientAsyncResponseReader<int>::operator new'
C:\src\op-del-example\op-del-example\op-del-example.cpp(38): note: see reference to function template instantiation 'grpc::ClientAsyncResponseReader<int> *grpc::ClientAsyncResponseReader<int>::Create<int>(grpc::ChannelInterface *,grpc::CompletionQueue *,const grpc::RpcMethod &,grpc::ClientContext *,const W &)' being compiled
        with
        [
            W=int
        ]
C:\src\op-del-example\op-del-example\op-del-example.cpp(33): note: see reference to function template instantiation 'grpc::ClientAsyncResponseReader<int> *grpc::ClientAsyncResponseReader<int>::Create<int>(grpc::ChannelInterface *,grpc::CompletionQueue *,const grpc::RpcMethod &,grpc::ClientContext *,const W &)' being compiled
        with
        [
            W=int
        ]

Anything else we should know about your project / environment?

ClientAsyncResponseReader appears to have mismatched placement new and placement delete operators as of commit 5845091#diff-beb45446397f2b7b25dc902290938075 by @ctiller.

operator new(std::size_t, T1, T2, ..., TN) needs to be matched with operator delete(void*, T1, T2, ... TN). So, the fix looks to be

diff --git a/include/grpc++/impl/codegen/async_unary_call.h b/include/grpc++/impl/codegen/async_unary_call.h
index aadf77d8a82..2b3a585801d 100644
--- a/include/grpc++/impl/codegen/async_unary_call.h
+++ b/include/grpc++/impl/codegen/async_unary_call.h
@@ -103,8 +103,7 @@ class ClientAsyncResponseReader final
   }
 
   // always allocated against a call arena, no memory free required
-  static void operator delete(void* ptr, std::size_t size) {
-    assert(size == sizeof(ClientAsyncResponseReader));
+  static void operator delete(void*, void*) {
   }
 
   /// See \a ClientAsyncResponseReaderInterface::ReadInitialMetadata for

NB: I'm not currently in the position to submit this as a PR, hence the patch above.

[vjpai]

I believe that this is not a problem. delete(void*, void*) is only called to free memory if new fails. As you see in the comments for delete in general, there is no need to free memory if new fails because this object is only ever allocated against an arena that is part of the underlying call (and that goes away when the call fails).

The delete(void, size_t) being defined here is for use when the object is explicitly deleted or deleted as part of the operation of a managed pointer, so it is needed (even though it again does nothing).

That said, if a supported compiler is throwing an unneeded warning, we could add an empty function for this case. It seems like an overly zealous warning, though. I'm wondering why our test builds haven't triggered it since we build using VS 2015.

[chwarr]

I'm able to see the same build warning from the CMake build target "grpc++_test_util" using master as of commit 796d81b:

PS> cmake S:\src\grpc\ -DgRPC_BUILD_TESTS=TRUE -G 'Visual Studio 14 2015 Win64'
...
PS>  cmake --build . --target grpc++_test_util -- /m
...
         S:\src\grpc\include\grpc++/impl/codegen/async_unary_call.h(102): warning C4291: 'void *grpc::ClientAsyncResponseReader<grpc::testing::EchoResponse>::operator new(std::size_t,void
       *)': no matching operator delete found; memory will not be freed if initialization throws an exception [S:\src\build-grpc\x64\vs\grpc++_test_util.vcxproj]

There's a similar warning coming from async_stream.h as well:

S:\src\grpc\include\grpc++/impl/codegen/async_stream.h(187): warning C4291: 'void *operator new(std::size_t,void *) throw()': no matching operator delete found; memory will not be freed if initialization throws an exception [S:\src\build-grpc\x64\vs\grpc++_test_util.vcxproj]

I suspect that it isn't failing in CI builds, as warnings don't appear to be treated as errors in the CMake-based build.

And, yes, this is one of those warnings from MSVC where it tells you that it's following the standard. If the constructor is marked as noexcept this warning is not emitted. Maybe that's a more robust way to fix this...

I care about this because my consumers build /W4 /WX /sdl, so my code needs to be similarly clean. I can #pragma warning(push); #pragma warning(disable: 4291); #pragma warning(pop) around the gRPC++ #include to make forward progress.

What is the target level of warning "cleanliness" for gRPC/gRPC++ built with MSVC?

Updated patch, if you just want to apply that:

diff --git a/include/grpc++/impl/codegen/async_unary_call.h b/include/grpc++/impl/codegen/async_unary_call.h
index aadf77d8a8..4e39fa8972 100644
--- a/include/grpc++/impl/codegen/async_unary_call.h
+++ b/include/grpc++/impl/codegen/async_unary_call.h
@@ -106,6 +106,8 @@ class ClientAsyncResponseReader final
   static void operator delete(void* ptr, std::size_t size) {
     assert(size == sizeof(ClientAsyncResponseReader));
   }
+  static void operator delete(void*, void*) {
+  }

   /// See \a ClientAsyncResponseReaderInterface::ReadInitialMetadata for
   /// semantics.