Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

Serialize node invocationInfo JSON directly to stream to avoid running out of memory #314

Merged
merged 2 commits into from
Oct 5, 2016
Merged

Serialize node invocationInfo JSON directly to stream to avoid running out of memory #314

merged 2 commits into from
Oct 5, 2016

Conversation

bradchristensen
Copy link
Contributor

Fixes sending invocation info to the node process when it is first created for the SocketNodeInstance, when serializing >30MB of JSON text and using 32-bit IIS Express (with a .NET Framework 4.5.2 app), which would otherwise result in an OutOfMemoryException at the GetBytes method.

This fix serializes the invocationInfo into a JSON string directly to the stream without double-handling it as a string to reduce the memory footprint of the application. It uses the NewtonsoftJson JsonTextWriter to handle serialization to the stream.

The underlying stream is kept open while the writers are disposed by creating the StreamWriter with 'true' as the last parameter of its constructor (this also requires specifying the buffer size, so I've copied the size used in a method below), and setting the CloseOutput property of the JsonWriter to false.

The cancellationToken is no longer passed through as the method is no longer async - JsonSerializer.serialize doesn't seem to have an async variant. I'm guessing this would have a slight performance impact, so maybe there's a better way of doing this?

Brad Christensen added 2 commits September 16, 2016 18:45
…g out of memory

Fixed only for SocketNodeInstance, as it deals nicely with streams. Previously ~30MB of JSON text and 32-bit IIS Express would result in an OutOfMemoryException at the GetBytes method, which is now fixed by writing the JSON string directly to the stream and not handling it as a string in between.
@dnfclas
Copy link

dnfclas commented Sep 16, 2016

Hi @bradchristensen, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

@SteveSandersonMS
Copy link
Member

Thanks - this looks good! I'll investigate in detail and see if there's anything we can do about the cancellation stuff when I get chance.

@SteveSandersonMS
Copy link
Member

Well, it's awkward that we'll have to block the .NET request-handling thread while the RPC message is being transferred to Node, but since there's no properly async version of JsonSerializer.Serialize(...), it looks like there's no better way. Even MVC's JsonResultExecutor uses this same technique (i.e., it's not actually async in the thread-releasing sense that you'd expect).

Fortunately we'll only be blocking the thread during the time it takes to send the RPC call to a socket that's typically on the same machine (and will not be blocking during the time we wait for Node to respond), so it's much less of an issue than it is for JsonResultExecutor, which is communicating with an arbitrary client across the internet.

So, your solution looks like the best solution - thanks for contributing it!

@SteveSandersonMS SteveSandersonMS merged commit 4fc1d60 into aspnet:dev Oct 5, 2016
@bradchristensen
Copy link
Contributor Author

Thanks so much for taking the time to look into this @SteveSandersonMS! Great idea to look at the MVC implementation - it's comforting to see that this lines up with what's been done there already.

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

Successfully merging this pull request may close these issues.

3 participants