Merge lp://qastaging/~vanvugt/mir/detect-overlapping-clients into lp://qastaging/mir

8 + if (ncompositors > 1 && nclients > 0 && !nfree())
16 + BOOST_THROW_EXCEPTION(std::logic_error(

Since the client is in no position to know about compositors, this cannot be a logic error from the client's perspective.

Is there a valid use case for giving to the client more than 1 buffer at any time? If not, then I think it should be cleaner to disallow it explicitly and have: if (nclients > 0) ..., which is more suitable as a client logic error.

Yes, there is a valid use case for allowing multiple client buffers. That is to pipeline rendering asynchronously and improve performance. It's worth considering for later. And I would rather not impose more arbitrary limitations...

> It's worth considering for later. And I
> would rather not impose more arbitrary limitations...

In that case, I don't think we should impose any limitation at all. The proposed limitation effectively says:
"we allow clients to get buffers until they are blocked on a request, but if a condition, which the client can't know about, is true inside the server, getting a buffer will cause the request (and the client) to fail".

Note the condition:
    (ncompositors > 1 && nclients > 0 && !nfree())

This means we still can support multiple client buffers, so long as we remember to create enough buffers in the first place (e.g. for bypass support and nclients==2, then nbuffers=4).

Also, I'm not asking the client to know anything. It's the _server_ that satisfies the precondition by:
  (a) Allocating enough buffers to the bundle; and
  (b) Controlling where the client buffer is released (before the next is acquired); and
  (c) Controlling whether bypass is supported (overlapping compositor buffers).

If your server is bug-free then it's fine. If your server contains a bug (like it did) then it will throw an exception. Client logic never factors into it.

> This means we still can support multiple client buffers, so long as we remember to create
> enough buffers in the first place

[snip]

> It's the _server_ that satisfies the precondition by:

How will the frontend code (the client from the BufferBundle's perspective) know when to stop requesting buffers in order not to fail? The current mechanism involves the frontend asking for a buffer from the surface, expecting to block if it can't get one. With the proposed restriction, requesting a client buffer can either block or fail, but the frontend won't be able to tell which is going to happen.

Although the proposed restriction will work for now, it won't work cleanly if we add support for multiple client buffers. We might as well be more explicit/restrictive and disallow multiple client buffer acquisitions completely (current precondition (b) requires it anyway) for a cleaner BundleBuffer behavior, until we support the feature properly.

Anyway, since this doesn't break anything currently, I won't block.

Note that I would still prefer not having a restriction at all, because of the strange (IMO) BufferBundle API behavior that the restriction introduces. I think having proper tests for SessionMediator is enough.

> Note that I would still prefer not having a restriction at all, because of the
> strange (IMO) BufferBundle API behavior that the restriction introduces. I
> think having proper tests for SessionMediator is enough.

I think the strange BufferBundle et al behavior is what needs fixing

BufferBundle should give out a single-owner, move-only type, not the shared_ptr we have now. Not only is shared ownership inappropriate it leads to the current code needing to release ownership everywhere the handle has been copied before calling flag_for_render(). This distributes responsibility around the system and is hard to police.

While this MP does a trap for this error I'd rather change the design to localize the knowledge required to avoid problems.

With the changes to BufferBundle outlined above we could localize the knowledge by changing "advance_client_buffer()" to "swap_buffer(BufferHandle&)" and enforce that the handle is released (and the buffer returned to the bundle) before calling flag_for_render().

Calling swap_buffer(BufferHandle&) with a null handle would avoid the messy "surface_in_startup" flag and also support the possible allocation of multiple buffers without spurious calls to flag_for_render().

Alexandros:
If by "fail" you mean throwing an exception, that will _never_happen_ if libmirserver remains bug free. An exception tells us we have regressed like in bug 1249210. Such a failure is a fatal error resulting in a crash report, which we then act upon.

"Although the proposed restriction will work for now, it won't work cleanly if we add support for multiple client buffers". See previous comments about why that is untrue. Multiple client buffers are still supported providing you allocate a big enough bundle. It's that simple.

I think we're drifting off topic now, or at least wasting time. My goal was to provide as good a regression test for bug 1249210 as possible. We presently have none. But I agree arbitrary limitations are not ideal and people may reject it on that basis.