Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Json errror in watch #828

Closed
zhiweiv opened this issue Apr 11, 2022 · 22 comments · Fixed by #835
Closed

Json errror in watch #828

zhiweiv opened this issue Apr 11, 2022 · 22 comments · Fixed by #835

Comments

@zhiweiv
Copy link
Contributor

zhiweiv commented Apr 11, 2022

The kubernetes API server closes watch connection after 30~60 minutes if nothing returned, in this case the client gets json deserialize error.

I am wondering if we can change the line https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/Watcher.cs#L182 from line == null to string.IsNullOrEmpty(line) to fix this.

The full exception stack here:

Microsoft.Rest.SerializationException: Unable to deserialize the response.
 ---> System.Text.Json.JsonException: The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. Path: $ | LineNumber: 0 | BytePositionInLine: 0.
 ---> System.Text.Json.JsonReaderException: The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. LineNumber: 0 | BytePositionInLine: 0.
   at System.Text.Json.ThrowHelper.ThrowJsonReaderException(Utf8JsonReader& json, ExceptionResource resource, Byte nextByte, ReadOnlySpan`1 bytes)
   at System.Text.Json.Utf8JsonReader.Read()
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   --- End of inner exception stack trace ---
   at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, JsonReaderException ex)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](JsonReaderState& readerState, Boolean isFinalBlock, ReadOnlySpan`1 buffer, JsonSerializerOptions options, ReadStack& state, JsonConverter converterBase)
   at System.Text.Json.JsonSerializer.ContinueDeserialize[TValue](ReadBufferState& bufferState, JsonReaderState& jsonReaderState, ReadStack& readStack, JsonConverter converter, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.ReadAll[TValue](Stream utf8Json, JsonTypeInfo jsonTypeInfo)
   at k8s.Kubernetes.CreateResultAsync[T](HttpRequestMessage httpRequest, HttpResponseMessage httpResponse, Nullable`1 watch, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at k8s.Kubernetes.CreateResultAsync[T](HttpRequestMessage httpRequest, HttpResponseMessage httpResponse, Nullable`1 watch, CancellationToken cancellationToken)
   at k8s.Kubernetes.ListPodForAllNamespacesWithHttpMessagesAsync(Nullable`1 allowWatchBookmarks, String continueParameter, String fieldSelector, String labelSelector, Nullable`1 limit, Nullable`1 pretty, String resourceVersion, String resourceVersionMatch, Nullable`1 timeoutSeconds, Nullable`1 watch, IDictionary`2 customHeaders, CancellationToken cancellationToken)
   at k8s.WatcherExt.<>c__DisplayClass1_0`2.<<MakeStreamReaderCreator>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at k8s.Watcher`1.<>c.<CreateWatchEventEnumerator>b__21_1[TR](Task`1 t)
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location ---
   at k8s.Watcher`1.CreateWatchEventEnumerator(Func`1 streamReaderCreator, Action`1 onError, CancellationToken cancellationToken)+MoveNext()
   at k8s.Watcher`1.CreateWatchEventEnumerator(Func`1 streamReaderCreator, Action`1 onError, CancellationToken cancellationToken)+System.Threading.Tasks.Sources.IValueTaskSource<System.Boolean>.GetResult()
   at k8s.Watcher`1.WatcherLoop(CancellationToken cancellationToken)
   at k8s.Watcher`1.WatcherLoop(CancellationToken cancellationToken)
@tg123
Copy link
Member

tg123 commented Apr 12, 2022

did you catch what returned from server side?

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 12, 2022

There is no chance to get it, the exception was thrown in internal code here https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/Watcher.cs#L191, so I guess it maybe an empty string.

@tg123
Copy link
Member

tg123 commented Apr 13, 2022

i believe it is something else like <html>, we'd better to throw the ex with original content.

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 13, 2022

After further check the exception stack, the exception is thrown here: https://github.com/kubernetes-client/csharp/blob/master/src/KubernetesClient/Kubernetes.cs#L89, before the previous location.

The exception only occur when nothing returned at all since watch request started.

@tg123
Copy link
Member

tg123 commented Apr 13, 2022

lets try to do wireshark or tcpdump to see if it is server side err

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 14, 2022

I will do more tests soon.

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 15, 2022

I figured out the problem, for the code

using (Stream stream = await httpResponse.Content.ReadAsStreamAsync().ConfigureAwait(false))
#endif
{
result.Body = KubernetesJson.Deserialize<T>(stream);
}

When watch without any result, the request hang here util the api server close the connection, the json deserialize from empty stream will throw exception.

Since the result.Body is not used in watch case, I think we can change to code to following to fix this issue

if (!watch.GetValueOrDefault())
{
    result.Body = KubernetesJson.Deserialize<T>(stream);
}

I have verified in local the watch works without json exception anymore.

Another way to fix is change stream to string before json deserialize

var content = await httpResponse.Content.ReadAsStringAsync().ConfigureAwait(false);
if (content != null)
{
    result.Body = KubernetesJson.Deserialize<T>(content);
}

@tg123
Copy link
Member

tg123 commented Apr 15, 2022

the internal stream is replaced by LineSeparatedHttpContent and content should returned from PeekFirstLine

checked code here

return _inner.ReadLineAsync();

should throw EOF if null

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 18, 2022

In the later code after the change I made, it gets string first before json deserialize, and return if string null, so the watch can finish without exception in watch no result case.

var line = await AttachCancellationToken(streamReader.ReadLineAsync()).ConfigureAwait(false);
cancellationToken.ThrowIfCancellationRequested();
if (line == null)
{
yield break;
}

@tg123
Copy link
Member

tg123 commented Apr 18, 2022

please drop a PR

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 18, 2022

I will create PR soon after I figure out another problem(watch hang in version 7.1+, 7.0.15 is ok).

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 19, 2022

There are two way to avoid json deserialize error in following code while watch no result

#if NET5_0_OR_GREATER
using (Stream stream = await httpResponse.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false))
#else
using (Stream stream = await httpResponse.Content.ReadAsStreamAsync().ConfigureAwait(false))
#endif
{
result.Body = KubernetesJson.Deserialize<T>(stream);
}

  1. Skip body in watch as it is not used in watch case, but this will introduce breaking change, for example client.ListNode(watch: true) will return null
if (!watch)
{
    result.Body = KubernetesJson.Deserialize<T>(stream);
}
  1. Use string instead of stream, I am not sure if it is performance consideration to use stream.
var content = await httpResponse.Content.ReadAsStringAsync().ConfigureAwait(false);
if (content != null)
{
    result.Body = KubernetesJson.Deserialize<T>(content);
}

@tg123
Which way you prefer to fix this problem?

@tg123
Copy link
Member

tg123 commented Apr 19, 2022

maybe to throw en eof exception

at

var line = await ReadLineAsync().ConfigureAwait(false);

when peekline === null, which means it is eof and nothing returned from server in the first line

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 20, 2022

If throw EOFException in LineSeparatedHttpContent, do you mean we catch EOFException like following

try
{
    using (Stream stream = await httpResponse.Content.ReadAsStreamAsync().ConfigureAwait(false))
    {
        result.Body = KubernetesJson.Deserialize<T>(stream);
    }
}
catch (JsonException)
{
    httpRequest.Dispose();
    httpResponse.Dispose();
    throw;
}
catch (EofException) //catch eof 
{
    httpRequest.Dispose();
    httpResponse.Dispose();
}
return result;

@tg123
Copy link
Member

tg123 commented Apr 20, 2022

I think throw the exception, because the read does not finish

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 20, 2022

You mean throw the exception outside of KubernetesClient, the client which uses this library to handle exception, right?

@tg123
Copy link
Member

tg123 commented Apr 20, 2022

yes, the eof exception is far clear than json deserialize

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 20, 2022

After throw EndOfStreamException in LineSeparatedHttpContent.SerializeToStreamAsync, the HttpContent encapsulate it to HttpRequestException, the exception stack looks like this, the client need to aware it from ex.InnerException, is it ok for you?

System.Net.Http.HttpRequestException: Error while copying content to a stream.
 ---> System.IO.EndOfStreamException: Attempted to read past the end of the stream.
   at k8s.LineSeparatedHttpContent.SerializeToStreamAsync(Stream stream, TransportContext context)
   at System.Net.Http.HttpContent.LoadIntoBufferAsyncCore(Task serializeToStreamTask, MemoryStream tempBuffer)

@tg123
Copy link
Member

tg123 commented Apr 20, 2022

Thanks I am open to it

any reason to wrap it with httprequestexception?

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 20, 2022

It is HttpContent who wrap it, it is .net inner logic, because LineSeparatedHttpContent extends HttpContent.

@tg123
Copy link
Member

tg123 commented Apr 20, 2022

It is HttpContent who wrap it, it is .net inner logic, because LineSeparatedHttpContent extends HttpContent.

+1 make sense

@zhiweiv
Copy link
Contributor Author

zhiweiv commented Apr 25, 2022

Verified in 7.2.19, I can better handle the exception with following code

Action<Exception> enError = e =>
{
    if (e is EndOfStreamException || e.InnerException is EndOfStreamException)
    {
        //Watch closed by api server, ignore this exception
    }
    else
    {
        Log($"Watch error, {e}");
    }
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants