mgravell / Pipelines.Sockets.Unofficial

.NET managed sockets wrapper using the new "Pipelines" API
Other
400 stars 51 forks source link

How do I flush synchronously? #61

Closed RayKoopa closed 3 years ago

RayKoopa commented 3 years ago

TLDR; I noticed I seemingly have to call SocketConnection.Output.FlushAsync to get messages sent to the client, but all of my code is synchronous. How do I flush synchronously, or can I just use _ = SocketConnection.Output.FlushAsync().Result?

Longer version: I have a sample length prefixed protocol writing messages to a client like this:

class MyMessage
{
    public string Text { get; set; }
}

// somewhere create a SocketConnection from an accepted Socket:
SocketConnection connection = SocketConection.Create(acceptedSocket);

// somewhere send a message:
WriteMessage(connection.Output, new() { Text = "Blablabla" });

void WriteMessage(MyMessage message, IBufferWriter<byte> output)
{
    int textLength = message.Text.GetLength();
    int messageLength = sizeof(int) + textLength;
    Span<byte> span = output.GetSpan(messageLength);

    BinaryPrimitives.WriteInt32BigEndian(span, textLength);
    Encoding.ASCII.GetBytes(message.Text, span[sizeof(int)..]);

    output.Advance(messageLength);
}

Using this, the client never receives this message, despite my call to connection.Output.Advance(messageLength), which I thought would be enough to kick off sending data.

I experimented around a bit and noticed that if I append a _ = client.Connection.Output.FlushAsync().Result to the end of the SendMessage method, it starts to receive messages.

That looks like an ugly hack as I did not find a way to flush synchronously. Am I missing something here or can I just go with this?

mgravell commented 3 years ago

I'm very rarely going to actively advocate calling sync-over-async, and this is no exception. If it works for you, fine - but: on your head be it. Ultimately, the answer here is "pipelines is designed as an async API; there is no synchronous flush". The intent of FlushAsync() here is for implementing back-pressure, i.e. where your writes are overtaking the limits that you've configured. When you flush, what it mostly does is make sure that the data is being actively written, but if you're over the limits, it also has the chance to say "just wait a moment until the data clears down a bit" - it will then resume the writer when there is obvious capacity in the pipe.

On a nearly empty pipe, you won't really see a problem from sync-over-async, as it will almost always be synchronous anyway; however, when you start hitting the limits (i.e. there's a backlog in the write queue), you'll find yourself blocking the writer thread(s), which could start a "spiral of death" if all the blocked writers means that there's no available pool threads to do the writing.

On your head be it.

RayKoopa commented 3 years ago

Thanks for the fast reply. I'll look into reworking this kinda legacy code base then to support async properly, I just wanted to ensure I'm not missing something before I delve into that.

(It is indeed not the first time I have problems with my legacy sync code - I initially tried to use the StreamPipeReader provided by the System.IO.Pipelines package, but its TryRead does not even ever return true unless one called ReadAsync before, which is why I tried your library as it happens to work in that case.)

Kaelum commented 2 years ago

@RayKoopa I know this is closed, but you should read up on how to properly use the "async" system. You should never use any property or method of an async call, when calling from a synchronous method. The proper action in this case would be:

Task.Run(() => client.Connection.Output.FlushAsync()).Wait();

Calling any async method and attempting to access any property or method directly from it directly, while inside a synchronous property or method, can cause a threading deadlock at any time.

RayKoopa commented 2 years ago

@Kaelum Thanks, I'm aware of this; the project which lacked proper support for asynchrony for which I opened this issue has since been reworked/rewritten to be fully asnyc and no longer requires a workarounds.

mgravell commented 2 years ago

using Task.Run(...).Wait() would retain much of the wrongness; arguably marginally better, but a small margin

On Wed, 1 Sep 2021, 17:58 William Bosacker, @.***> wrote:

@RayKoopa https://github.com/RayKoopa I know this is closed, but you should read up on how to properly use the "async" system. You should never use any property or method of an async call, when calling from a synchronous method. The proper action in this case would be:

Task.Run(() => client.Connection.Output.FlushAsync()).Wait();

Calling any async method and attempting to access any property or method directly from it directly, while inside a synchronous property or method, can cause a threading deadlock at any time.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mgravell/Pipelines.Sockets.Unofficial/issues/61#issuecomment-910473058, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMDPRKIUZL7TS7W7AXDT7ZLRVANCNFSM43OPSLZQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.