OSDN Git Service

Protect MIDI framework against client blocks in MidiReceiver.onSend
authorMikhail Naganov <mnaganov@google.com>
Fri, 8 Jul 2016 20:26:19 +0000 (13:26 -0700)
committerMikhail Naganov <mnaganov@google.com>
Wed, 13 Jul 2016 20:50:17 +0000 (13:50 -0700)
Make the server-side socket non-blocking when creating MidiOutputPort
for clients. Thus if a client ceases to read from its side of the
socket pair, the server will just fail to write instead of blocking.

One drawback is that the MidiOutputPort on the client can't indicate
that it has become dysfunctional, but it's not possible without
changing the API.

Bug: 29413812
Change-Id: I9dfcbdd214a815cea8fd1365324fd78ca459268a

core/java/com/android/internal/midi/MidiDispatcher.java
media/java/android/media/midi/MidiDeviceServer.java
media/java/android/media/midi/MidiOutputPort.java

index 1a3c37c..c16628a 100644 (file)
@@ -26,11 +26,23 @@ import java.util.concurrent.CopyOnWriteArrayList;
  * Utility class for dispatching MIDI data to a list of {@link android.media.midi.MidiReceiver}s.
  * This class subclasses {@link android.media.midi.MidiReceiver} and dispatches any data it receives
  * to its receiver list. Any receivers that throw an exception upon receiving data will
- * be automatically removed from the receiver list, but no IOException will be returned
- * from the dispatcher's {@link android.media.midi.MidiReceiver#onSend} in that case.
+ * be automatically removed from the receiver list. If a MidiReceiverFailureHandler has been
+ * provided to the MidiDispatcher, it will be notified about the failure, but the exception
+ * itself will be swallowed.
  */
 public final class MidiDispatcher extends MidiReceiver {
 
+    // MidiDispatcher's client and MidiReceiver's owner can be different
+    // classes (e.g. MidiDeviceService is a client, but MidiDeviceServer is
+    // the owner), and errors occuring during sending need to be reported
+    // to the owner rather than to the sender.
+    //
+    // Note that the callbacks will be called on the sender's thread.
+    public interface MidiReceiverFailureHandler {
+        void onReceiverFailure(MidiReceiver receiver, IOException failure);
+    }
+
+    private final MidiReceiverFailureHandler mFailureHandler;
     private final CopyOnWriteArrayList<MidiReceiver> mReceivers
             = new CopyOnWriteArrayList<MidiReceiver>();
 
@@ -46,6 +58,14 @@ public final class MidiDispatcher extends MidiReceiver {
         }
     };
 
+    public MidiDispatcher() {
+        this(null);
+    }
+
+    public MidiDispatcher(MidiReceiverFailureHandler failureHandler) {
+        mFailureHandler = failureHandler;
+    }
+
     /**
      * Returns the number of {@link android.media.midi.MidiReceiver}s this dispatcher contains.
      * @return the number of receivers
@@ -70,8 +90,13 @@ public final class MidiDispatcher extends MidiReceiver {
             try {
                 receiver.send(msg, offset, count, timestamp);
             } catch (IOException e) {
-                // if the receiver fails we remove the receiver but do not propagate the exception
+                // If the receiver fails we remove the receiver but do not propagate the exception.
+                // Note that this may also happen if the client code stalls, and thus underlying
+                // MidiInputPort.onSend has raised IOException for EAGAIN / EWOULDBLOCK error.
                 mReceivers.remove(receiver);
+                if (mFailureHandler != null) {
+                    mFailureHandler.onReceiverFailure(receiver, e);
+                }
             }
         }
     }
@@ -79,7 +104,15 @@ public final class MidiDispatcher extends MidiReceiver {
     @Override
     public void onFlush() throws IOException {
        for (MidiReceiver receiver : mReceivers) {
-            receiver.flush();
+            try {
+                receiver.flush();
+            } catch (IOException e) {
+                // This is just a special case of 'send' thus handle in the same way.
+                mReceivers.remove(receiver);
+                if (mFailureHandler != null) {
+                    mFailureHandler.onReceiverFailure(receiver, e);
+                }
+            }
        }
     }
 }
index f0abf71..fbc8615 100644 (file)
@@ -73,6 +73,10 @@ public final class MidiDeviceServer implements Closeable {
 
     private final Callback mCallback;
 
+    private final HashMap<IBinder, PortClient> mPortClients = new HashMap<IBinder, PortClient>();
+    private final HashMap<MidiInputPort, PortClient> mInputPortClients =
+            new HashMap<MidiInputPort, PortClient>();
+
     public interface Callback {
         /**
          * Called to notify when an our device status has changed
@@ -102,6 +106,10 @@ public final class MidiDeviceServer implements Closeable {
 
         abstract void close();
 
+        MidiInputPort getInputPort() {
+            return null;
+        }
+
         @Override
         public void binderDied() {
             close();
@@ -152,9 +160,12 @@ public final class MidiDeviceServer implements Closeable {
             mInputPorts.remove(mInputPort);
             IoUtils.closeQuietly(mInputPort);
         }
-    }
 
-    private final HashMap<IBinder, PortClient> mPortClients = new HashMap<IBinder, PortClient>();
+        @Override
+        MidiInputPort getInputPort() {
+            return mInputPort;
+        }
+    }
 
     // Binder interface stub for receiving connection requests from clients
     private final IMidiDeviceServer mServer = new IMidiDeviceServer.Stub() {
@@ -215,6 +226,10 @@ public final class MidiDeviceServer implements Closeable {
                 ParcelFileDescriptor[] pair = ParcelFileDescriptor.createSocketPair(
                                                     OsConstants.SOCK_SEQPACKET);
                 MidiInputPort inputPort = new MidiInputPort(pair[0], portNumber);
+                // Configure the server-side socket in non-blocking mode to avoid stalling
+                // the entire MIDI framework if client app code gets stuck inside 'onSend'
+                // handler.
+                IoUtils.setBlocking(pair[0].getFileDescriptor(), false);
                 MidiDispatcher dispatcher = mOutputPortDispatchers[portNumber];
                 synchronized (dispatcher) {
                     dispatcher.getSender().connect(inputPort);
@@ -228,6 +243,9 @@ public final class MidiDeviceServer implements Closeable {
                 synchronized (mPortClients) {
                     mPortClients.put(token, client);
                 }
+                synchronized (mInputPortClients) {
+                    mInputPortClients.put(inputPort, client);
+                }
                 return pair[1];
             } catch (IOException e) {
                 Log.e(TAG, "unable to create ParcelFileDescriptors in openOutputPort");
@@ -237,12 +255,19 @@ public final class MidiDeviceServer implements Closeable {
 
         @Override
         public void closePort(IBinder token) {
+            MidiInputPort inputPort = null;
             synchronized (mPortClients) {
                 PortClient client = mPortClients.remove(token);
                 if (client != null) {
+                    inputPort = client.getInputPort();
                     client.close();
                 }
             }
+            if (inputPort != null) {
+                synchronized (mInputPortClients) {
+                    mInputPortClients.remove(inputPort);
+                }
+            }
         }
 
         @Override
@@ -270,6 +295,9 @@ public final class MidiDeviceServer implements Closeable {
             synchronized (mPortClients) {
                 mPortClients.put(token, client);
             }
+            synchronized (mInputPortClients) {
+                mInputPortClients.put(inputPort, client);
+            }
             return Process.myPid(); // for caller to detect same process ID
         }
 
@@ -303,7 +331,7 @@ public final class MidiDeviceServer implements Closeable {
 
         mOutputPortDispatchers = new MidiDispatcher[numOutputPorts];
         for (int i = 0; i < numOutputPorts; i++) {
-            mOutputPortDispatchers[i] = new MidiDispatcher();
+            mOutputPortDispatchers[i] = new MidiDispatcher(mInputPortFailureHandler);
         }
 
         mInputPortOpen = new boolean[mInputPortCount];
@@ -312,6 +340,20 @@ public final class MidiDeviceServer implements Closeable {
         mGuard.open("close");
     }
 
+    private final MidiDispatcher.MidiReceiverFailureHandler mInputPortFailureHandler =
+            new MidiDispatcher.MidiReceiverFailureHandler() {
+                public void onReceiverFailure(MidiReceiver receiver, IOException failure) {
+                    Log.e(TAG, "MidiInputPort failed to send data", failure);
+                    PortClient client = null;
+                    synchronized (mInputPortClients) {
+                        client = mInputPortClients.remove(receiver);
+                    }
+                    if (client != null) {
+                        client.close();
+                    }
+                }
+            };
+
     // Constructor for MidiDeviceService.onCreate()
     /* package */ MidiDeviceServer(IMidiManager midiManager, MidiReceiver[] inputPortReceivers,
            MidiDeviceInfo deviceInfo, Callback callback) {
index 0096995..54c31e3 100644 (file)
@@ -83,7 +83,7 @@ public final class MidiOutputPort extends MidiSender implements Closeable {
                 }
             } catch (IOException e) {
                 // FIXME report I/O failure?
-                Log.e(TAG, "read failed");
+                Log.e(TAG, "read failed", e);
             } finally {
                 IoUtils.closeQuietly(mInputStream);
             }