Mir

Code review comment for lp:~raof/mir/release-rendersurface-may-require-RPC

Revision history for this message
Chris Halse Rogers (raof) wrote :

Releasing streams requires an RPC wait for two reasons:
1) It's a necessary client synchronisation point: clients almost always want to have some data associated with a Mir object, and they can't free that associated data until we can guarantee they'll never get a callback on that Mir object again.
2) The code
mir_render_surface_release(rs)
mir_connection_release(connection)
is currently racy, and will frequently lead to client deadlocks.

Like so:
  Both the rs_release and connection_release client requests will get queued up
  to the socket, the server sends the responses and then closes the connection.
  .
  The rs_release response results in erasing the bufferstream from the connection
  map.
  .
  The bufferstream destructor calls release_buffer on all of its allocated buffers.
  (This is why the current acceptance tests don't notice the problem - we don't
   wait for the bufferstream to actually be valid, so its allocation requests
   are unfulfilled at destruction time, so it never tries to release them.)
  .
  The buffer_release call hits the RPC layer, which notices that the socket has
  been closed and fires on_disconnected.
  .
  on_disconnected runs all the pending completions. The first one is...
  release_connection, which immediately tries to grab the connection_map
  lock, which we already hold way up in rs_release, and deadlock.

I guess both of these could be solved instead by
a) Ensuring that mir_render_surface_release does not return until it's guaranteed that no more callbacks will be fired on the underlying bufferstream, and
b) Ensuring that we don't deadlock in this scenario.

I'll see how difficult this is to do.

« Back to merge proposal