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

Provide a way of not closing brackets when an exception occurs during serialization. #1134

Closed
kichalla opened this issue Jan 5, 2017 · 6 comments

Comments

@kichalla
Copy link

kichalla commented Jan 5, 2017

Related to aspnet/Mvc#5413
On investigating the above issue we found that not closing the json writer in case of exceptions would result in any rented buffers to be not returned to the array pool. And any workarounds on MVC side to fix this seems to be dealing with internal details of Json.NET which feels incorrect.

Proposal:
Could JsonSerializerSettings expose an option to either close(default)/not-close the brackets on exception? Based on the option, could JsonSerializerInternalWriter poison the state of the writer it's using so that the writer knows not to close the brackets since an exception has occurred?

@Eilon

@JamesNK
Copy link
Owner

JamesNK commented Jan 5, 2017

I don't want to add a setting to JsonSerializerSettings for something so niche but I could add a flag to JsonTextWriter.

@kichalla
Copy link
Author

kichalla commented Jan 5, 2017

That sounds good!

@Eilon
Copy link

Eilon commented Jan 6, 2017

@JamesNK cool. Do you see this feature making its way in at any point in the near future? Anything we can do to help?

@JamesNK
Copy link
Owner

JamesNK commented Jan 10, 2017

It isn't difficult, just a bool property to control what happens when JsonTextWriter is disposed/closed.

I don't know when the next release will be - the number of changes is turning out to be huge and there isn't a pressing need. I'll probably wait until after VS2017 is out and I can move over to the csproj.

@JamesNK
Copy link
Owner

JamesNK commented Jan 19, 2017

Done - 55afb9a

FYI I might change the property name if I can think of something I like more

@JamesNK JamesNK closed this as completed Jan 19, 2017
@Eilon
Copy link

Eilon commented Jan 20, 2017

@JamesNK very cool, thanks!

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

No branches or pull requests

3 participants