kerryjiang / SuperSocket

SuperSocket is a light weight, cross platform and extensible socket server application framework.
Apache License 2.0
3.95k stars 1.15k forks source link

客户端同时大量发送和接收数据时报错: Writing is not allowed after writer was completed. #706

Closed wuuer closed 6 months ago

wuuer commented 8 months ago

测试代码: ConsoleApp1.zip

BOLL7708 commented 6 months ago

Can confirm that I also ended up getting this, although so far it seems a bit random, I've sent a total of more than 5+ million messages using this code before this happened, will keep an eye out if it happens again.

Application: OpenVR2WS.exe
CoreCLR Version: 8.0.224.6711
.NET Version: 8.0.2
Description: The process was terminated due to an unhandled exception.
Exception Info: System.InvalidOperationException: Writing is not allowed after writer was completed.
   at System.IO.Pipelines.ThrowHelper.ThrowInvalidOperationException_NoWritingAllowed()
   at SuperSocket.WebSocket.WebSocketEncoder.Encode(IBufferWriter`1 writer, WebSocketPackage pack)
   at SuperSocket.Connection.PipeConnectionBase.WritePackageWithEncoder[TPackage](IBufferWriter`1 writer, IPackageEncoder`1 packageEncoder, TPackage package)
   at SuperSocket.Connection.PipeConnectionBase.SendAsync[TPackage](IPackageEncoder`1 packageEncoder, TPackage package, CancellationToken cancellationToken)
   at EasyFramework.SuperServer.SendMessage(WebSocketSession session, String message) in D:\Google Drive\-= BOLL7708 =-\Rider\OpenVR2WS\EasyFramework\SuperServer.cs:line 120
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.QueueUserWorkItemCallback.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

Here is the referenced line where it crashed.

kerryjiang commented 6 months ago

Will test it out

BOLL7708 commented 6 months ago

I'm pondering if it is something about sending a message to a session just after it has connected, as I do that with some default welcome data, and I just got the same crash again just when reloading a browser based client.

I put the SendAsync() call in a try/catch and kept reloading said client page, and about 8 rapid reloads in I managed to get this again. The try/catch block keeps the server and application that runs it alive though, so I'll probably add a few retries to handle this on my end in any case. 😁

kerryjiang commented 6 months ago

@wuuer For your case, after I attached a console logger to the client, I got the error message:

Package cannot be larger than 1048576

It seems you didn't append the package terminator in end of the message when you echo back messages to client:

await s.SendAsync(System.Text.Encoding.UTF8.GetBytes(p)); 

==>

await s.SendAsync(System.Text.Encoding.UTF8.GetBytes(p + "\r\n")); 
kerryjiang commented 6 months ago

@BOLL7708 There are something wrong in your code IMO:

1) there might be endless broadcast issue in this piece of code? Take a look at this code path: SendAllMessage() => SendMessage() => SendAllMessage().

2) you use async void as the signature of the method SendMessage(). In this case, you better handle exceptions by yourself or the process will crash. In would suggest you to change the method to async Task, and do concurrent await in SendAllMessage() if you want to get threads, cpu under control, and whole logic operations under control.

image
BOLL7708 commented 6 months ago

The if statement on line 118 will fail if session is null, and then run SendMessageToAll(), which will only send messages to sessions that are not null. So in theory, not an infinite loop, but I can see how it is dangerous and I'll look this over per your recommendation.

As a side note, I wrote most of this two years ago when I was upgrading to .NET 6 for my projects, but then I got side-tracked but now I'm back at it and upgrading to .NET 8, this with a different IDE, Rider. This IDE suggested switching the if statement in question to use is which I understand but it does hide the null check so I'm not sure it was a good idea for readability 😅

Cheers!

wuuer commented 6 months ago

@wuuer For your case, after I attached a console logger to the client, I got the error message:

Package cannot be larger than 1048576

It seems you didn't append the package terminator in end of the message when you echo back messages to client:

await s.SendAsync(System.Text.Encoding.UTF8.GetBytes(p)); 

==>

await s.SendAsync(System.Text.Encoding.UTF8.GetBytes(p + "\r\n")); 

That is it!

BOLL7708 commented 6 months ago

I did improve my code now, I think, it handles 100+ clients and thousands of messages in a short span of time and rapidly reconnecting clients in my load test, no issues at all. Cheers again and thanks for your work 😁