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

Replace SimpleJson usage with System.Text.Json #6770

Merged
merged 4 commits into from
Jun 15, 2023
Merged

Conversation

danielmarbach
Copy link
Contributor

@danielmarbach danielmarbach commented Jun 14, 2023

Replaces the previous usage of SimpleJson in two places with System.Text.Json:

  • Diagnostics file output
  • Learning Persistence saga storage

We have replaced in a number of downstream the simplejson usages as well and so far, they have been frictionless. For the learning persistence I don't think we give any guarantees, so if there is a breaking change (which is quite unlikely) then that wouldn't be the end of the world since it is not a production persister.

The startup diagnostic files get overwritten on each start so that is also not a problem.

@andreasohlund andreasohlund added this to the 8.1.0 milestone Jun 14, 2023
@DavidBoike
Copy link
Member

@danielmarbach any theories on the test failures?

@danielmarbach
Copy link
Contributor Author

One is the encoding of the diagnostic entries. The other one don't know yet

@danielmarbach
Copy link
Contributor Author

For some reasons I couldn't figure out yet it seems we are now exposed to race problems on the saga persister.

@DavidBoike
Copy link
Member

Saga issues were because System.Text.Json serializer does not "clip" the end of a file's output when you are writing to a read/write FileStream. You have to do fileStream.SetLength(fileStream.Position) after writing or you could have leftover characters from the previous contents of the file.

@DavidBoike DavidBoike merged commit 38e154a into master Jun 15, 2023
2 checks passed
@DavidBoike DavidBoike deleted the simplejson branch June 15, 2023 16:21
@danielmarbach
Copy link
Contributor Author

Awesome. Nice find!

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

Successfully merging this pull request may close these issues.

None yet

3 participants