ligang/vpp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

VPP-533 Fix ping race condition in JVpp

Browse Source 
Improper synchronization between ping_send and ping_reply_handle

Change-Id: I844c96bc3f5cd750a1c43188d3133c92f8f14e38
Signed-off-by: Maros Marsalek <mmarsale@cisco.com>
This commit is contained in:
Maros Marsalek
2016-11-14 16:14:58 +01:00
committed by Damjan Marion
parent c0f6cf36a5
commit 94ea8b7ff3
1 changed files with 28 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
vpp-api/java/jvpp-registry/io/fd/vpp/jvpp/JVppRegistryImpl.java
View File

@ -25,7 +25,6 @@ import java.io.IOException;
import java.util.HashMap; import java.util.HashMap;
import java.util.Map; import java.util.Map;
import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.logging.Level; import java.util.logging.Level;
import java.util.logging.Logger; import java.util.logging.Logger;
@ -37,14 +36,16 @@ public final class JVppRegistryImpl implements JVppRegistry, ControlPingCallback
private static final Logger LOG = Logger.getLogger(JVppRegistryImpl.class.getName()); private static final Logger LOG = Logger.getLogger(JVppRegistryImpl.class.getName());
private final VppJNIConnection connection; private final VppJNIConnection connection;
// Unguarded concurrent map, no race conditions expected on top of that
private final Map<String, JVppCallback> pluginRegistry; private final Map<String, JVppCallback> pluginRegistry;
private final ConcurrentMap<Integer, ControlPingCallback> pingCalls; // Guarded by self
private final Map<Integer, ControlPingCallback> pingCalls;
public JVppRegistryImpl(final String clientName) throws IOException { public JVppRegistryImpl(final String clientName) throws IOException {
connection = new VppJNIConnection(clientName); connection = new VppJNIConnection(clientName);
connection.connect(); connection.connect();
pluginRegistry = new HashMap<>(); pluginRegistry = new ConcurrentHashMap<>();
pingCalls = new ConcurrentHashMap<>(); pingCalls = new HashMap<>();
} }
@Override @Override
@ -53,7 +54,7 @@ public final class JVppRegistryImpl implements JVppRegistry, ControlPingCallback
} }
@Override @Override
public synchronized void register(final JVpp jvpp, final JVppCallback callback) { public void register(final JVpp jvpp, final JVppCallback callback) {
requireNonNull(jvpp, "jvpp should not be null"); requireNonNull(jvpp, "jvpp should not be null");
requireNonNull(callback, "Callback should not be null"); requireNonNull(callback, "Callback should not be null");
final String name = jvpp.getClass().getName(); final String name = jvpp.getClass().getName();
@ -67,14 +68,14 @@ public final class JVppRegistryImpl implements JVppRegistry, ControlPingCallback
} }
@Override @Override
public synchronized void unregister(final String name) { public void unregister(final String name) {
requireNonNull(name, "Plugin name should not be null"); requireNonNull(name, "Plugin name should not be null");
final JVppCallback previous = pluginRegistry.remove(name); final JVppCallback previous = pluginRegistry.remove(name);
assertPluginWasRegistered(name, previous); assertPluginWasRegistered(name, previous);
} }
@Override @Override
public synchronized JVppCallback get(final String name) { public JVppCallback get(final String name) {
requireNonNull(name, "Plugin name should not be null"); requireNonNull(name, "Plugin name should not be null");
JVppCallback value = pluginRegistry.get(name); JVppCallback value = pluginRegistry.get(name);
assertPluginWasRegistered(name, value); assertPluginWasRegistered(name, value);
@ -84,27 +85,33 @@ public final class JVppRegistryImpl implements JVppRegistry, ControlPingCallback
private native int controlPing0() throws VppInvocationException; private native int controlPing0() throws VppInvocationException;
@Override @Override
public synchronized int controlPing(final Class<? extends JVpp> clazz) throws VppInvocationException { public int controlPing(final Class<? extends JVpp> clazz) throws VppInvocationException {
connection.checkActive(); connection.checkActive();
final String name = clazz.getName(); final String name = clazz.getName();
final ControlPingCallback callback = (ControlPingCallback) pluginRegistry.get(clazz.getName()); final ControlPingCallback callback = (ControlPingCallback) pluginRegistry.get(clazz.getName());
assertPluginWasRegistered(name, callback); assertPluginWasRegistered(name, callback);
int context = controlPing0(); synchronized (pingCalls) {
if (context < 0) { int context = controlPing0();
throw new VppInvocationException("controlPing", context); if (context < 0) {
} throw new VppInvocationException("controlPing", context);
}
pingCalls.put(context, callback); pingCalls.put(context, callback);
return context; return context;
}
} }
@Override @Override
public void onControlPingReply(final ControlPingReply reply) { public void onControlPingReply(final ControlPingReply reply) {
final ControlPingCallback callback = pingCalls.get(reply.context); final ControlPingCallback callback;
synchronized (pingCalls) {
callback = pingCalls.remove(reply.context);
}
if (callback == null) { if (callback == null) {
LOG.log(Level.WARNING, "No callback was registered for reply id={0} ", reply.context); LOG.log(Level.WARNING, "No callback was registered for reply context=" + reply.context + " Contexts waiting="
+ pingCalls.keySet());
return; return;
} }
// pass the reply to the callback registered by the ping caller // pass the reply to the callback registered by the ping caller
@ -114,7 +121,11 @@ public final class JVppRegistryImpl implements JVppRegistry, ControlPingCallback
@Override @Override
public void onError(final VppCallbackException ex) { public void onError(final VppCallbackException ex) {
final int ctxId = ex.getCtxId(); final int ctxId = ex.getCtxId();
final ControlPingCallback callback = pingCalls.get(ctxId); final ControlPingCallback callback;
synchronized (pingCalls) {
callback = pingCalls.get(ctxId);
}
if (callback == null) { if (callback == null) {
LOG.log(Level.WARNING, "No callback was registered for reply id={0} ", ctxId); LOG.log(Level.WARNING, "No callback was registered for reply id={0} ", ctxId);
return; return;