Solve some safety issues with RPCs (#1127)

Java would never properly dispose, and C++'s were easy to respond after disposing.
We now return a bool if the call was successful or not.

ntcore/src/main/java/edu/wpi/first/networktables/NetworkTableInstance.java

@@ -566,6 +566,7 @@ public final class NetworkTableInstance implements AutoCloseable {
                   + throwable.toString());
               throwable.printStackTrace();
             }
+            event.finish();
           }
         }
       }

ntcore/src/main/java/edu/wpi/first/networktables/NetworkTablesJNI.java

@@ -139,7 +139,7 @@ public final class NetworkTablesJNI {
   public static native RpcAnswer[] pollRpcTimeout(NetworkTableInstance inst, int poller, double timeout) throws InterruptedException;
   public static native void cancelPollRpc(int poller);
   public static native boolean waitForRpcCallQueue(int inst, double timeout);
-  public static native void postRpcResponse(int entry, int call, byte[] result);
+  public static native boolean postRpcResponse(int entry, int call, byte[] result);
   public static native int callRpc(int entry, byte[] params);
   public static native byte[] getRpcResult(int entry, int call);
   public static native byte[] getRpcResult(int entry, int call, double timeout);

ntcore/src/main/java/edu/wpi/first/networktables/RpcAnswer.java

@@ -10,7 +10,7 @@ package edu.wpi.first.networktables;
 /**
  * NetworkTables Remote Procedure Call (Server Side).
  */
-public final class RpcAnswer implements AutoCloseable {
+public final class RpcAnswer {
   /** Entry handle. */
   @SuppressWarnings("MemberName")
   public final int entry;
@@ -52,18 +52,14 @@ public final class RpcAnswer implements AutoCloseable {
 
   static final byte[] emptyResponse = new byte[] {};
 
-  @Deprecated
-  public void free() {
-    close();
-  }
-
-  /**
-   * Posts an empty response if one was not previously sent.
+  /*
+   * Finishes an RPC answer by replying empty if the user did not respond.
+   * Called internally by the callback thread.
    */
-  @Override
-  public synchronized void close() {
+  void finish() {
     if (call != 0) {
-      postResponse(emptyResponse);
+      NetworkTablesJNI.postRpcResponse(entry, call, emptyResponse);
+      call = 0;
     }
   }
 
@@ -78,10 +74,12 @@ public final class RpcAnswer implements AutoCloseable {
   /**
    * Post RPC response (return value) for a polled RPC.
    * @param result  result raw data that will be provided to remote caller
+   * @return        true if the response was posted, otherwise false
    */
-  public void postResponse(byte[] result) {
-    NetworkTablesJNI.postRpcResponse(entry, call, result);
+  public boolean postResponse(byte[] result) {
+    boolean ret = NetworkTablesJNI.postRpcResponse(entry, call, result);
     call = 0;
+    return ret;
   }
 
   /* Network table instance. */

ntcore/src/main/native/cpp/RpcServer.cpp

@@ -35,14 +35,15 @@ void RpcServer::ProcessRpc(unsigned int local_id, unsigned int call_uid,
        send_response);
 }
 
-void RpcServer::PostRpcResponse(unsigned int local_id, unsigned int call_uid,
+bool RpcServer::PostRpcResponse(unsigned int local_id, unsigned int call_uid,
                                 wpi::StringRef result) {
   auto thr = GetThread();
   auto i = thr->m_response_map.find(impl::RpcIdPair{local_id, call_uid});
   if (i == thr->m_response_map.end()) {
     WARNING("posting RPC response to nonexistent call (or duplicate response)");
-    return;
+    return false;
   }
   (i->getSecond())(result);
   thr->m_response_map.erase(i);
+  return true;
 }

ntcore/src/main/native/cpp/RpcServer.h

@@ -99,7 +99,7 @@ class RpcServer : public IRpcServer,
                   SendResponseFunc send_response,
                   unsigned int rpc_uid) override;
 
-  void PostRpcResponse(unsigned int local_id, unsigned int call_uid,
+  bool PostRpcResponse(unsigned int local_id, unsigned int call_uid,
                        wpi::StringRef result);
 
  private:

ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp

@@ -1255,17 +1255,17 @@ Java_edu_wpi_first_networktables_NetworkTablesJNI_waitForRpcCallQueue
 /*
  * Class:     edu_wpi_first_networktables_NetworkTablesJNI
  * Method:    postRpcResponse
- * Signature: (II[B)V
+ * Signature: (II[B)Z
  */
-JNIEXPORT void JNICALL
+JNIEXPORT jboolean JNICALL
 Java_edu_wpi_first_networktables_NetworkTablesJNI_postRpcResponse
   (JNIEnv* env, jclass, jint entry, jint call, jbyteArray result)
 {
   if (!result) {
     nullPointerEx.Throw(env, "result cannot be null");
-    return;
+    return false;
   }
-  nt::PostRpcResponse(entry, call, JByteArrayRef{env, result});
+  return nt::PostRpcResponse(entry, call, JByteArrayRef{env, result});
 }
 
 /*

ntcore/src/main/native/cpp/ntcore_c.cpp

@@ -431,9 +431,9 @@ NT_Bool NT_WaitForRpcCallQueue(NT_Inst inst, double timeout) {
   return nt::WaitForRpcCallQueue(inst, timeout);
 }
 
-void NT_PostRpcResponse(NT_Entry entry, NT_RpcCall call, const char* result,
-                        size_t result_len) {
-  nt::PostRpcResponse(entry, call, StringRef(result, result_len));
+NT_Bool NT_PostRpcResponse(NT_Entry entry, NT_RpcCall call, const char* result,
+                           size_t result_len) {
+  return nt::PostRpcResponse(entry, call, StringRef(result, result_len));
 }
 
 NT_RpcCall NT_CallRpc(NT_Entry entry, const char* params, size_t params_len) {

ntcore/src/main/native/cpp/ntcore_cpp.cpp

@@ -560,18 +560,18 @@ bool WaitForRpcCallQueue(NT_Inst inst, double timeout) {
   return ii->rpc_server.WaitForQueue(timeout);
 }
 
-void PostRpcResponse(NT_Entry entry, NT_RpcCall call, StringRef result) {
+bool PostRpcResponse(NT_Entry entry, NT_RpcCall call, StringRef result) {
   Handle handle{entry};
   int id = handle.GetTypedIndex(Handle::kEntry);
   auto ii = InstanceImpl::Get(handle.GetInst());
-  if (id < 0 || !ii) return;
+  if (id < 0 || !ii) return false;
 
   Handle chandle{call};
   int call_uid = chandle.GetTypedIndex(Handle::kRpcCall);
-  if (call_uid < 0) return;
-  if (handle.GetInst() != chandle.GetInst()) return;
+  if (call_uid < 0) return false;
+  if (handle.GetInst() != chandle.GetInst()) return false;
 
-  ii->rpc_server.PostRpcResponse(id, call_uid, result);
+  return ii->rpc_server.PostRpcResponse(id, call_uid, result);
 }
 
 NT_RpcCall CallRpc(NT_Entry entry, StringRef params) {

ntcore/src/main/native/include/ntcore_c.h

@@ -851,9 +851,10 @@ NT_Bool NT_WaitForRpcCallQueue(NT_Inst inst, double timeout);
  * @param call        RPC call handle (from NT_RpcAnswer)
  * @param result      result raw data that will be provided to remote caller
  * @param result_len  length of result in bytes
+ * @return            true if the response was posted, otherwise false
  */
-void NT_PostRpcResponse(NT_Entry entry, NT_RpcCall call, const char* result,
-                        size_t result_len);
+NT_Bool NT_PostRpcResponse(NT_Entry entry, NT_RpcCall call, const char* result,
+                           size_t result_len);
 
 /**
  * Call a RPC function.  May be used on either the client or server.

ntcore/src/main/native/include/ntcore_cpp.h

@@ -14,6 +14,7 @@
 #include <functional>
 #include <memory>
 #include <string>
+#include <thread>
 #include <utility>
 #include <vector>
 
@@ -132,7 +133,7 @@ class RpcAnswer {
   NT_Entry entry;
 
   /** Call handle. */
-  NT_RpcCall call;
+  mutable NT_RpcCall call;
 
   /** Entry name. */
   std::string name;
@@ -152,8 +153,9 @@ class RpcAnswer {
   /**
    * Post RPC response (return value) for a polled RPC.
    * @param result  result raw data that will be provided to remote caller
+   * @return True if posting the response is valid, otherwise false
    */
-  void PostResponse(StringRef result) const;
+  bool PostResponse(StringRef result) const;
 
   friend void swap(RpcAnswer& first, RpcAnswer& second) {
     using std::swap;
@@ -936,8 +938,9 @@ bool WaitForRpcCallQueue(NT_Inst inst, double timeout);
  * @param entry       entry handle of RPC entry (from RpcAnswer)
  * @param call        RPC call handle (from RpcAnswer)
  * @param result      result raw data that will be provided to remote caller
+ * @return            true if the response was posted, otherwise false
  */
-void PostRpcResponse(NT_Entry entry, NT_RpcCall call, StringRef result);
+bool PostRpcResponse(NT_Entry entry, NT_RpcCall call, StringRef result);
 
 /**
  * Call a RPC function.  May be used on either the client or server.
@@ -1480,8 +1483,10 @@ bool WaitForLoggerQueue(NT_Inst inst, double timeout);
 
 /** @} */
 
-inline void RpcAnswer::PostResponse(StringRef result) const {
-  PostRpcResponse(entry, call, result);
+inline bool RpcAnswer::PostResponse(StringRef result) const {
+  auto ret = PostRpcResponse(entry, call, result);
+  call = 0;
+  return ret;
 }
 
 }  // namespace nt