Add ServerCloseMessage to protocol #236
Conversation
zackliu
commented
Oct 19, 2018
zackliu
commented
- As customer can't get any error messages when service terminate the server connections now, it will be very confusing when service reach message limit. Under that circumstance, server will keep reconnecting without any useful reasons.
- ServerCloseMessage is used to send back error messages from service before service abort the connection. There are some changes in service to enable the workflow and in SDK, we just log these error messages.
- This is not a breaking change, old sdk can work with new service and vice versa.
| return new MultiGroupBroadcastDataMessage(groupList, payloads); | ||
| } | ||
|
|
||
| private static ServerCloseMessage createServerCloseMessage(byte[] input, ref int offset) |
There was a problem hiding this comment.
Fix the casing of this method, it should pascal cased.
| binary: "kw6Spmdyb3VwNKZncm91cDWCpGpzb27ECAECAwQFBgcIq21lc3NhZ2VwYWNrxAgHCAECAwQFBg=="), | ||
| new ProtocolTestData( | ||
| name: "ServerClose", | ||
| message: new ServerCloseMessage("Message count reach limit: 100000"), |
There was a problem hiding this comment.
This error message should be: "Maximum message count limit reached: 100000"
|
Where's the unit test? Does it stop reconnecting as a result? |
|
No it won't, just log an error message. |
|
Where does the message show up? In the logs? Can you ever recover from this error or will it keep getting sent until a day has passed? |
|
It will show in the log. We can't recover it in the SDK side, we just log it and reconnect. The reasons of aborting connection are various, some of them can be recovered after reconnecting. For those can't be recovered through reconnecting e.g. message count limit reached, we will handle it in service side and turn to a slow reconnecting path.(#233) |
|
I'm thinking that it would be good to capture this exception and re-throw it from the calls to . The user would get the message from the service side of a generic message. |
|
This function is to write message to service side. But now we get error message from service and soon the transport connection and application connection will be terminated. We have no way to throw it to user. |
That's not true. This is directly called by user code here https://github.com/Azure/azure-signalr/blob/c823e993aba01774e6f19faf78f216e8b71f8e46/src/Microsoft.Azure.SignalR/HubHost/ServiceLifetimeManager.cs. If we capture the exception and rethrow it, the user will be able to observe it in their code. |
|
I see, but we receive error message in |
By storing the error and throwing it when WriteAsync is called. When the connection recovers the error should be cleared. |
It dosn't need to be one or the other, I don't see why you wouldn't do both. I'd also probably make this a specific exception type. |
|
I think throwing exception when calling After we receives ServerCloseMessage, the connection will be terminated soon. And this error should be only the connection scoped error. So when a new connection is created, we can't treat it as a bad connection. |
That's my point though. You can give the user the right error message instead of the generic one "The connection is not active, data cannot be sent to the service." becomes the actual error that came from the service. |
|
I love not throwing generic one "The connection is not active, data cannot be sent to the service."! |
| case ServiceProtocolConstants.MultiGroupBroadcastDataMessageType: | ||
| return CreateMultiGroupBroadcastDataMessage(input, ref startOffset); | ||
| case ServiceProtocolConstants.ServerCloseMessageType: | ||
| return createServerCloseMessage(input, ref startOffset); |
There was a problem hiding this comment.
Please make the function name consistent with others style: "CreateServerCloseMessage"
| return MultiGroupBroadcastDataMessagesEqual(multiGroupBroadcastDataMessage, | ||
| (MultiGroupBroadcastDataMessage)y); | ||
| case ServerCloseMessage serverCloseMessage: | ||
| return ServerCloseMessageEqual(serverCloseMessage, (ServerCloseMessage) y); |
There was a problem hiding this comment.
nit: remove the white space in type casting: "(ServerCloseMessage) y);"
| private bool _isStopped; | ||
| private long _lastReceiveTimestamp; | ||
| protected ConnectionContext _connection; | ||
| protected string _errorMessage; |
There was a problem hiding this comment.
Any reason this isn't private?
There was a problem hiding this comment.
Inheritance class which override WriteAsync can use _errorMessage
| throw new Exception(serverCloseMessage.ErrorMessage); | ||
| } | ||
|
|
||
| return Task.CompletedTask; |
There was a problem hiding this comment.
CompletedTask [](start = 24, length = 13)
shall we close the server connection with this message?
| if (!string.IsNullOrEmpty(serverCloseMessage.ErrorMessage)) | ||
| { | ||
| _errorMessage = serverCloseMessage.ErrorMessage; | ||
| throw new Exception(serverCloseMessage.ErrorMessage); |
There was a problem hiding this comment.
Exception [](start = 26, length = 9)
throw a more detailed exception?
| { | ||
| if (!string.IsNullOrEmpty(serviceErrorMessage.ErrorMessage)) | ||
| { | ||
| // When receives service error message, we suppose server -> service connection doesn't work, |
There was a problem hiding this comment.
@davidfowl After discussed offline, we renamed the message name and changed to log rather than throw exceptions in ProcessIncomingAsync
| LoggerMessage.Define(LogLevel.Warning, new EventId(26, "FailedSendingPing"), "Failed sending a ping message to service."); | ||
|
|
||
| private static readonly Action<ILogger, string, Exception> _receivedServiceErrorMessage = | ||
| LoggerMessage.Define<string>(LogLevel.Warning, new EventId(27, "ReceivedServiceErrorMessage"), "Received error message from service: {Error}"); |
|
|
||
| if (!string.IsNullOrEmpty(ErrorMessage)) | ||
| { | ||
| _serviceConnectionLock.Release(); |
There was a problem hiding this comment.
use a local errorMessage=ErrorMessage for throw, ErrorMessage can be reset in between if and throw
4 participants
