[PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

[Sebastian Lackner]

On 28.02.2017 18:10, Jacek Caban wrote:
> On 28.02.2017 16:44, Sebastian Lackner wrote:
>> Well, there are also situations where it would be relatively easy to avoid
>> the roundtrip. We could return immediately at least when no events, APCs or
>> completions are involved, right? Since this is the most common situation it
>> might still be worth the effort.
>
> If event is not passed, we still need to signal object itself. Thinking
> about it some more, I guess we could just signal object it before
> returning for non-overlapped operations returning immediately. It
> wouldn't be exactly right, but it can't be used for synchronization
> reliably in this case anyway.
> 
> Another thing is that we currently don't know if there is user APC until
> APC_ASYNC_IO returns, but that can be fixed.
> 
> Since it's just an optimization, can we process with the patch itself
> first? I'm happy to optimize that, but I find it hard to agree that an
> optimization should block inclusion of patches targeted at correctness.

I'm not saying that it should block anything, I will leave the design decision
up to Alexandre. Nevertheless, I think it certainly makes sense to discuss
about such aspects in advance. Depending on the solution used it might also
require refactoring of this patch, which is easier to do before it was accepted.

>> For flush we are currently not using any callback, so marking it in the handler
>> would be too late. Thats also something we could fix of course - Nevertheless,
>> as you can see it already gets pretty complicated. ;) If its possible to
>> simplify this somehow, that would certainly be an improvement.
> I think you misunderstood me somehow. I can only guess what you mean,
> since current flush() implementation queue async or return no wait
> handle - they never call async_terminate() from what I can tell. Even if
> they did, it would work with what I proposed:

Sorry if the previous message wasn't clear. Yes, I was thinking about the
hypothetical situation that we have to call async_terminate from a flush handler.

> flush handler creates async - it's marked as returning immediately
> flush calls async_terminate() - due to above, async_terminate is aware
> that it's an immediate result
> flush op returns to request handler, which finds that there was an
> immediate return, so does not mark async as returning asynchronously.

Yes, something like that would be necessary.

> 
> I'm not sure what you expect me to improve in this regards. This is
> about hypothetical solution to a problem that's not strictly related to
> this patch. And I don't think it's that complicated ;)

Well, my main suggestion is that it would be better to handle async returns
different than sync returns. Especially, we probably shouldn't use APC_ASYNC_IO
as a way to delay async_set_result for synchronous returns. Also, we should use
the reply buffer for read calls, or replace the wine_server_set_reply with a
length argument if this is not feasible.

Best regards,
Sebastian