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

Async methods in JsonConvert should be removed #66

Closed
svick opened this Issue Mar 26, 2013 · 31 comments

Comments

Projects
None yet
@svick

svick commented Mar 26, 2013

Because of a question on SO, I went to investigate how does async work in JSON.NET. When I looked at the code (specifically JsonConvert.cs), I noticed that the async methods (like SerializeObjectAsync()) just call the synchronous version using Task.Factory.StartNew().

This is generally not a good idea, because Async methods are usually more scalable than their synchronous version, but this isn't true for your implementation. For more information (and a better explanation), see Stephen Toub's article Should I expose asynchronous wrappers for synchronous methods?

Because of that, I think you should remove the Async versions of your methods from the library.

Where adding Async versions would make sense is when working with streams, but doing so would require a lot of changes in classes like JsonTextReader (and I'm not sure how to do it without duplicating a lot of code in a way that doesn't break JSON.NET for older versions of .Net).

@ejball

This comment has been minimized.

Show comment
Hide comment
@ejball

ejball commented Mar 27, 2013

+1

@Athari

This comment has been minimized.

Show comment
Hide comment
@Athari

Athari Mar 27, 2013

Contributor

Agreed, until true async versions are implemented, current "async" methods should be removed.

Contributor

Athari commented Mar 27, 2013

Agreed, until true async versions are implemented, current "async" methods should be removed.

@JamesNK

This comment has been minimized.

Show comment
Hide comment
@JamesNK

JamesNK May 5, 2013

Owner

I can't remove them because that will be a breaking change. I've added a note to the intelli-sense that serialization will happen on a new thread.

Edit: They're now marked as obsolete.

Owner

JamesNK commented May 5, 2013

I can't remove them because that will be a breaking change. I've added a note to the intelli-sense that serialization will happen on a new thread.

Edit: They're now marked as obsolete.

@JamesNK JamesNK closed this May 5, 2013

@markrendle

This comment has been minimized.

Show comment
Hide comment
@markrendle

markrendle Sep 11, 2013

Could I suggest running the serialization synchronously and returning a completed Task as a non-breaking change?

markrendle commented Sep 11, 2013

Could I suggest running the serialization synchronously and returning a completed Task as a non-breaking change?

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Nov 26, 2013

I agree that you can't remove these methods because some people probably use them, but I think you should mark them as deprecated.
Do you plan to provide real async support in JsonSerializer? That would be very useful when working with HttpClient (which has a fully asynchronous API). Currently there is no way to deserialize asynchronously directly from a stream, which can be an issue when deserializing large objects.

thomaslevesque commented Nov 26, 2013

I agree that you can't remove these methods because some people probably use them, but I think you should mark them as deprecated.
Do you plan to provide real async support in JsonSerializer? That would be very useful when working with HttpClient (which has a fully asynchronous API). Currently there is no way to deserialize asynchronously directly from a stream, which can be an issue when deserializing large objects.

@MisinformedDNA

This comment has been minimized.

Show comment
Hide comment
@MisinformedDNA

MisinformedDNA Jan 20, 2014

I agree that they need to be marked Obsolete.

They are also misleading because they share the naming convention of async/await and so they appear, at first glance, good candidates for improving scalability. They don't.

The intelli-sense doesn't help (as I just ran into this). "Deserialization will happen on a different thread" isn't enough. Without looking at the implementation, it's still could imply async/await.

If you don't want to mark them Obsolete right away, consider making the intelli-sense more direct like "WARNING: This will be marked Obsolete in a future release. Please use the synchronous version instead."

MisinformedDNA commented Jan 20, 2014

I agree that they need to be marked Obsolete.

They are also misleading because they share the naming convention of async/await and so they appear, at first glance, good candidates for improving scalability. They don't.

The intelli-sense doesn't help (as I just ran into this). "Deserialization will happen on a different thread" isn't enough. Without looking at the implementation, it's still could imply async/await.

If you don't want to mark them Obsolete right away, consider making the intelli-sense more direct like "WARNING: This will be marked Obsolete in a future release. Please use the synchronous version instead."

@Injac

This comment has been minimized.

Show comment
Hide comment
@Injac

Injac Mar 29, 2014

A dumb question: What to use instead? I am using (Assembly version) version 6.0.0.0? Any hints? Docs?

Injac commented Mar 29, 2014

A dumb question: What to use instead? I am using (Assembly version) version 6.0.0.0? Any hints? Docs?

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Mar 29, 2014

@Injac, you should use the synchronous version. If you need to offload it to a new thread, do it explicitly with Task.Run (but don't do that in a server app, because it won't scale).

thomaslevesque commented Mar 29, 2014

@Injac, you should use the synchronous version. If you need to offload it to a new thread, do it explicitly with Task.Run (but don't do that in a server app, because it won't scale).

@Injac

This comment has been minimized.

Show comment
Hide comment
@Injac

Injac Mar 29, 2014

Thanks for the tip. This is what I have done unfortunately. Well, next time better read the warnings in VS.

Injac commented Mar 29, 2014

Thanks for the tip. This is what I have done unfortunately. Well, next time better read the warnings in VS.

@Nearga

This comment has been minimized.

Show comment
Hide comment
@Nearga

Nearga Apr 21, 2014

I'm afraid, i didnt get the point about scalability (in the article http://blogs.msdn.com/b/pfxteam/archive/2012/03/24/10287244.aspx) while calling sync methods asyncly.

It is okay if they would "using a bit more, since there’s overhead incurred to scheduling something", unless ui is not blocked. For example, i have a fat-ass Json which i want to parse: it is better if parsing would take 2 seconds on the background thread than 1 second on the ui thread, isnt it?

Or is it purely about resource consumption, kinda "2 seconds of work is more than 1 second of work" ?

Nearga commented Apr 21, 2014

I'm afraid, i didnt get the point about scalability (in the article http://blogs.msdn.com/b/pfxteam/archive/2012/03/24/10287244.aspx) while calling sync methods asyncly.

It is okay if they would "using a bit more, since there’s overhead incurred to scheduling something", unless ui is not blocked. For example, i have a fat-ass Json which i want to parse: it is better if parsing would take 2 seconds on the background thread than 1 second on the ui thread, isnt it?

Or is it purely about resource consumption, kinda "2 seconds of work is more than 1 second of work" ?

@Nearga

This comment has been minimized.

Show comment
Hide comment
@Nearga

Nearga Apr 21, 2014

And second question.

Here ( http://developer.nokia.com/community/wiki/Asynchronous_Programming_For_Windows_Phone_8 ) it is said that using TaskCompletionSource would be enough for making sync call async.

So, if i'll use

private async Task ExecuteAsync(RestRequest request)
{
var tcs = new TaskCompletionSource();
_client.ExecuteAsync(request, resp =>
{
var value = JsonConvert.DeserializeObject(resp.Content);
if (value.ErrorCode > 0)
{
var ex = new ToodledoException(value.ErrorCode, value.ErrorDesc);
tcs.SetException(ex);
}
else
tcs.SetResult(value);
});
return await tcs.Task;
}

would it be okay from both Scalability and Offloading from the previous article?

Nearga commented Apr 21, 2014

And second question.

Here ( http://developer.nokia.com/community/wiki/Asynchronous_Programming_For_Windows_Phone_8 ) it is said that using TaskCompletionSource would be enough for making sync call async.

So, if i'll use

private async Task ExecuteAsync(RestRequest request)
{
var tcs = new TaskCompletionSource();
_client.ExecuteAsync(request, resp =>
{
var value = JsonConvert.DeserializeObject(resp.Content);
if (value.ErrorCode > 0)
{
var ex = new ToodledoException(value.ErrorCode, value.ErrorDesc);
tcs.SetException(ex);
}
else
tcs.SetResult(value);
});
return await tcs.Task;
}

would it be okay from both Scalability and Offloading from the previous article?

@Athari

This comment has been minimized.

Show comment
Hide comment
@Athari

Athari Apr 21, 2014

Contributor

@Nearga You should only worry about scalability when you're writing an application which creates a lot of threads and consumes lots of CPU power. It's common for servers generating responses for thousands of clients. When you're writing a simple app for Windows Phone 8, you normally worry only about offloading.

Contributor

Athari commented Apr 21, 2014

@Nearga You should only worry about scalability when you're writing an application which creates a lot of threads and consumes lots of CPU power. It's common for servers generating responses for thousands of clients. When you're writing a simple app for Windows Phone 8, you normally worry only about offloading.

@Nearga

This comment has been minimized.

Show comment
Hide comment
@Nearga

Nearga Apr 21, 2014

@Athari Okay, but what about first question? And about whole topic in general? So, Async methods were removed because of crossplatform Newtonsoft.Json usage? It is ok to use

public static Task SerializeObjectAsync(object value, Formatting formatting, JsonSerializerSettings settings)
{
return Task.Factory.StartNew(() => SerializeObject(value, formatting, settings));
}

on the WP? Or should i add TaskCompletionSource like in previous post?

Nearga commented Apr 21, 2014

@Athari Okay, but what about first question? And about whole topic in general? So, Async methods were removed because of crossplatform Newtonsoft.Json usage? It is ok to use

public static Task SerializeObjectAsync(object value, Formatting formatting, JsonSerializerSettings settings)
{
return Task.Factory.StartNew(() => SerializeObject(value, formatting, settings));
}

on the WP? Or should i add TaskCompletionSource like in previous post?

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Apr 21, 2014

@Nearga, yes, it's OK to offload the serialization to another thread in a client app, in order to keep the UI thread responsive. But that's something you should do yourself, not something that should be in the API, because an async method suggests that the method will scale well (which is important for web apps, for instance), even though it won't (since it uses a thread).

thomaslevesque commented Apr 21, 2014

@Nearga, yes, it's OK to offload the serialization to another thread in a client app, in order to keep the UI thread responsive. But that's something you should do yourself, not something that should be in the API, because an async method suggests that the method will scale well (which is important for web apps, for instance), even though it won't (since it uses a thread).

@Nearga

This comment has been minimized.

Show comment
Hide comment
@Nearga

Nearga Apr 21, 2014

@thomaslevesque Thanks, now its clear.

So, async is for io operations (not blocking ui, running in the same thread) and Tasks are for cpu-heavy things (running in another thread)?

But how (in theory) to make the serialization well-scalable?

PS: sorry, edited question, restored it.

Nearga commented Apr 21, 2014

@thomaslevesque Thanks, now its clear.

So, async is for io operations (not blocking ui, running in the same thread) and Tasks are for cpu-heavy things (running in another thread)?

But how (in theory) to make the serialization well-scalable?

PS: sorry, edited question, restored it.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Apr 21, 2014

@Nearga, no. A Task is just a way to represent an operation that will complete at a later time; it could be running on a separate thread, or not; it could also just be waiting (in a non-blocking way) for the completion of an IO operation.

When keeping the UI responsive is the primary concern, CPU-heavy tasks should be offloaded to another thread.

But when all threads are equally important (such as in a server environment), you gain nothing by offloading to another thread, and you incur useless overhead. On the other hand, if a Task is using asynchronous IO, then it's not using a thread, so it's good for scalability.

thomaslevesque commented Apr 21, 2014

@Nearga, no. A Task is just a way to represent an operation that will complete at a later time; it could be running on a separate thread, or not; it could also just be waiting (in a non-blocking way) for the completion of an IO operation.

When keeping the UI responsive is the primary concern, CPU-heavy tasks should be offloaded to another thread.

But when all threads are equally important (such as in a server environment), you gain nothing by offloading to another thread, and you incur useless overhead. On the other hand, if a Task is using asynchronous IO, then it's not using a thread, so it's good for scalability.

@Nearga

This comment has been minimized.

Show comment
Hide comment
@Nearga

Nearga Apr 21, 2014

@thomaslevesque So, in general, well-scalable functions are just based on lowlevel async calls?

Kinda
Deserialize()
{


Reader.Read()


}

and "well-scalable" is

DeserializeAsync()
{


await Reader.ReadAsync()


}

Nearga commented Apr 21, 2014

@thomaslevesque So, in general, well-scalable functions are just based on lowlevel async calls?

Kinda
Deserialize()
{


Reader.Read()


}

and "well-scalable" is

DeserializeAsync()
{


await Reader.ReadAsync()


}

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Apr 21, 2014

@Nearga, basically, to make a method scalable, it must not do any blocking calls that cause the thread to be blocked doing nothing. When you call Stream.Read, the CPU does nothing until the IO operation completes, but the thread is still using resources; threads are a valuable resource on a server, you can't afford to waste them by making them wait uselessly... On the other hand, if you use Stream.ReadAsync, the thread can be reused to do something else while the IO operation completes.

thomaslevesque commented Apr 21, 2014

@Nearga, basically, to make a method scalable, it must not do any blocking calls that cause the thread to be blocked doing nothing. When you call Stream.Read, the CPU does nothing until the IO operation completes, but the thread is still using resources; threads are a valuable resource on a server, you can't afford to waste them by making them wait uselessly... On the other hand, if you use Stream.ReadAsync, the thread can be reused to do something else while the IO operation completes.

@Nearga

This comment has been minimized.

Show comment
Hide comment
@Nearga

Nearga Apr 21, 2014

@thomaslevesque Great, thanks. Now i understand, why it was removed.

Nearga commented Apr 21, 2014

@thomaslevesque Great, thanks. Now i understand, why it was removed.

@adrianm64

This comment has been minimized.

Show comment
Hide comment
@adrianm64

adrianm64 Oct 10, 2016

I don't get this.
Having SerializeAsync create threads is of course an anti-pattern but why was it removed? Couldn't it just call the Async methods on the underlying writer to get scalability? Currently the only solution to serialize async into a file/network is to store a temporary (i.e. MemoryStream) and then .CopyToAsync().

adrianm64 commented Oct 10, 2016

I don't get this.
Having SerializeAsync create threads is of course an anti-pattern but why was it removed? Couldn't it just call the Async methods on the underlying writer to get scalability? Currently the only solution to serialize async into a file/network is to store a temporary (i.e. MemoryStream) and then .CopyToAsync().

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Oct 10, 2016

Couldn't it just call the Async methods on the underlying writer to get scalability?

It could, but it's not that simple. I tried to do it a few years ago, but gave up after a while. Basically, the problem is that while the sync and async implementations are almost identical, it's hard to extract the common code, so you end up with almost duplicated implementations, which is hard to maintain. And in this case, you need to add async implementations all the way down to JsonReader.

In the typical case, JSON documents are not very large, so the benefit of an async implementation would be marginal, compared to the cost of writing and maintaining the implementation.

thomaslevesque commented Oct 10, 2016

Couldn't it just call the Async methods on the underlying writer to get scalability?

It could, but it's not that simple. I tried to do it a few years ago, but gave up after a while. Basically, the problem is that while the sync and async implementations are almost identical, it's hard to extract the common code, so you end up with almost duplicated implementations, which is hard to maintain. And in this case, you need to add async implementations all the way down to JsonReader.

In the typical case, JSON documents are not very large, so the benefit of an async implementation would be marginal, compared to the cost of writing and maintaining the implementation.

@fretje

This comment has been minimized.

Show comment
Hide comment
@fretje

fretje May 23, 2017

Now that async support is implemented, couldn't these methods be re-introduced but now with a real async implementation?

I'm not sure how to tackle this though...

I have a DeserializeObjectFromStream method like this:

public static T DeserializeObjectFromStream<T>(Stream stream, JsonSerializerSettings settings = null)
{
    var serializer = JsonSerializer.Create(settings);
    T data;
    using (var reader = new StreamReader(stream, Encoding.UTF8))
    {
        data = (T)serializer.Deserialize(reader, typeof(T));
    }
    return data;
}

How would I go about changing this to use the JObject.LoadAsync method, but still uses the provided serializersettings?

fretje commented May 23, 2017

Now that async support is implemented, couldn't these methods be re-introduced but now with a real async implementation?

I'm not sure how to tackle this though...

I have a DeserializeObjectFromStream method like this:

public static T DeserializeObjectFromStream<T>(Stream stream, JsonSerializerSettings settings = null)
{
    var serializer = JsonSerializer.Create(settings);
    T data;
    using (var reader = new StreamReader(stream, Encoding.UTF8))
    {
        data = (T)serializer.Deserialize(reader, typeof(T));
    }
    return data;
}

How would I go about changing this to use the JObject.LoadAsync method, but still uses the provided serializersettings?

@JamesNK

This comment has been minimized.

Show comment
Hide comment
@JamesNK

JamesNK May 23, 2017

Owner

The helper methods read and write strings which don't have any blocking IO. There is no point.

Owner

JamesNK commented May 23, 2017

The helper methods read and write strings which don't have any blocking IO. There is no point.

@fretje

This comment has been minimized.

Show comment
Hide comment
@fretje

fretje May 23, 2017

Thanks for the response! But what about that code example in my previous comment?
When the stream provided is a FileStream, there surely must be some blocking IO involved? Or am I missing something?

fretje commented May 23, 2017

Thanks for the response! But what about that code example in my previous comment?
When the stream provided is a FileStream, there surely must be some blocking IO involved? Or am I missing something?

@weitzhandler

This comment has been minimized.

Show comment
Hide comment
@weitzhandler

weitzhandler Dec 11, 2017

Async methods would still be useful when serializing to an IO resource.

weitzhandler commented Dec 11, 2017

Async methods would still be useful when serializing to an IO resource.

@TylerBrinkley

This comment has been minimized.

Show comment
Hide comment
@TylerBrinkley

TylerBrinkley Dec 11, 2017

Contributor

This is in regards to JsonConvert only, which only reads and writes strings. It doesn't have any stream reader/writer support. It would be nice though for async to make it's way to the JsonSerializer class.

Contributor

TylerBrinkley commented Dec 11, 2017

This is in regards to JsonConvert only, which only reads and writes strings. It doesn't have any stream reader/writer support. It would be nice though for async to make it's way to the JsonSerializer class.

@JonHanna

This comment has been minimized.

Show comment
Hide comment
@JonHanna

JonHanna Dec 11, 2017

Contributor

@TylerBrinkley I intend to make that my project for the holiday season.

Contributor

JonHanna commented Dec 11, 2017

@TylerBrinkley I intend to make that my project for the holiday season.

@petterton

This comment has been minimized.

Show comment
Hide comment
@petterton

petterton Feb 26, 2018

@JonHanna Any news on async support for the JsonSerializer class? This would be really useful!

petterton commented Feb 26, 2018

@JonHanna Any news on async support for the JsonSerializer class? This would be really useful!

@JonHanna

This comment has been minimized.

Show comment
Hide comment
@JonHanna

JonHanna Feb 26, 2018

Contributor

@petterton my OSS output is a bit down of late as my new son isn't as keen on me coding while he sleeps on my chest as my second-youngest was (my output actually went up when second-youngest was born, as I had long periods where coding was the only thing I could do, while he snuggled). It's still on my to-do list, but as I've less coding time right now nobody should see it as a licked cookie if they want to do it.

Contributor

JonHanna commented Feb 26, 2018

@petterton my OSS output is a bit down of late as my new son isn't as keen on me coding while he sleeps on my chest as my second-youngest was (my output actually went up when second-youngest was born, as I had long periods where coding was the only thing I could do, while he snuggled). It's still on my to-do list, but as I've less coding time right now nobody should see it as a licked cookie if they want to do it.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Feb 26, 2018

Now that JsonWriter and JsonReader support async operations, implementing SerializeAsync/DeserializeAsync shouldn't be too hard.

thomaslevesque commented Feb 26, 2018

Now that JsonWriter and JsonReader support async operations, implementing SerializeAsync/DeserializeAsync shouldn't be too hard.

@JonHanna

This comment has been minimized.

Show comment
Hide comment
@JonHanna

JonHanna Feb 26, 2018

Contributor

Not terribly. I've a start made, but haven't had a chance to do the necessary tests

Contributor

JonHanna commented Feb 26, 2018

Not terribly. I've a start made, but haven't had a chance to do the necessary tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment