Removes blocking calls to http stream when using Newtonsoft json #354
Conversation
|
I am not sure about the call to get stream length. This might or might not throw. I am keen to disable it unless somebody knows for sure if it is safe to use it. |
…to make safe call
src/Giraffe/Serialization.fs
Outdated
| sr.Serialize(jw, x) | ||
| Task.CompletedTask | ||
| let memoryStream = new MemoryStream() | ||
| let streamWriter = new StreamWriter(memoryStream, Utf8EncodingWithoutBom) |
There was a problem hiding this comment.
Should we be using the use keyword for the StreamWriter as it is disposable (same for the JsonTextWriter and the readers down below)?
There was a problem hiding this comment.
I was sure this is for disposal of underlying stream, in this case the stream is a resize array, but I will check
There was a problem hiding this comment.
Good catch, it flushes buffer on dispose, will introduce uses where they were
There was a problem hiding this comment.
Do those flushes need to be async too? See here: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#always-call-flushasync-on-streamwriters-or-streams-before-calling-dispose
There was a problem hiding this comment.
Yeah, needs to be adressed, great catch!
There was a problem hiding this comment.
There is no need in calling flush async here because writes to memory stream are synchronous anyway.
I've added manual flush before copy to the output though
There was a problem hiding this comment.
Cool, I thought that might be the case, I sometimes get in a middle about what should be flushed when, especially when there are multiple streams involved
|
Sorry to necro this, but the trade off here now is using the |
Untill true async apis are available from Newtonsoft itself, this is probably the way to go to be able to work with .net core 2.2+
Sumary of changes: do not allow newtonsoft json classes touch raw http stream, cache in memory stream first.
Addresses #351