CSHARP-1525: JsonBuffer can exhaust memory. #4
Conversation
| if (_jsonReaderSettings.AutoBufferReset && _bookmarks.Count == 0) | ||
| { | ||
| _buffer.ResetBuffer(); | ||
| } |
There was a problem hiding this comment.
Try to reset after reading a token (buffer will reset only if MinResetBufferSize chars read)
There was a problem hiding this comment.
There is no requirement that ReturnToBookmark be called (it might or it might not).
Therefore, there is no guarantee that the Count will ever go back to zero.
I think we need a new approach.
| _buffer.Position = jsonReaderBookmark.Position; | ||
|
|
||
| // TODO BD throw if not found? | ||
| _bookmarks.Remove(jsonReaderBookmark); |
There was a problem hiding this comment.
Show we disallow custom created bookmarks (maybe as an JsonReaderSettings option)?
| /// TODO BD | ||
| /// </summary> | ||
| public int MinResetBufferSize { get; set; } = 512; | ||
|
|
There was a problem hiding this comment.
Can we use an approach that doesn't involve adding any new public APIs?
There was a problem hiding this comment.
We can, but:
- Would not it be beneficial to provide tuning options for advanced JsonReader.cs users?
- Wanted to introduce "killswitch" with JsonReaderSettings.AutoBufferReset
There was a problem hiding this comment.
We have a "no knobs" principle at MongoDB.
In practice that really means "as few knobs as possible".
There was a problem hiding this comment.
I see, would we want to leave JsonReaderSettings.AutoBufferReset if we were to proceed with this approach?
There was a problem hiding this comment.
I haven't reviewed carefully besides pointing out that we would prefer not to change the public API at all if possible.
Can we find an approach that "just works" without requiring any public API changes?
There was a problem hiding this comment.
Yes, the suggested change should just work. JsonReaderSettings.AutoBufferReset is proposed for additional safety. Whether we can do without it depends on our confidence of the proposed changes.
There was a problem hiding this comment.
I think you should remove these new settings (knobs).
They are not required.
The ticket is about clearing the buffer when possible to reduce memory consumption, not about making more things configurable.
DmitryLukyanov
changed the title
DmitryLukyanov
left a comment
DmitryLukyanov
left a comment
There was a problem hiding this comment.
Initial review. I will take a look at the changes itself after Robert
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="JsonBuffer" /> class. | ||
| /// </summary> | ||
| /// <summary>Initializes a new instance of the <see cref="JsonBuffer" /> class.</summary> |
There was a problem hiding this comment.
let's use the default formatting for xml comment:
/// <summary>
/// ...
/// </summary>
| /// <summary>Initializes a new instance of the <see cref="JsonBuffer" /> class.</summary> | ||
| /// <param name="reader">The reader.</param> | ||
| public JsonBuffer(TextReader reader) | ||
| /// <param name="readChunkSize"></param> |
| /// <summary> | ||
| /// TODO BD | ||
| /// </summary> | ||
| public bool AutoBufferReset { get; set; } = true; |
There was a problem hiding this comment.
in the current styling guide, we don't use auto properties
There was a problem hiding this comment.
Also, properties should be placed after constructor
| /// TODO BD | ||
| /// </summary> | ||
| public int MinResetBufferSize { get; set; } = 512; | ||
|
|
There was a problem hiding this comment.
I think you should remove these new settings (knobs).
They are not required.
The ticket is about clearing the buffer when possible to reduce memory consumption, not about making more things configurable.
| if (_jsonReaderSettings.AutoBufferReset && _bookmarks.Count == 0) | ||
| { | ||
| _buffer.ResetBuffer(); | ||
| } |
There was a problem hiding this comment.
There is no requirement that ReturnToBookmark be called (it might or it might not).
Therefore, there is no guarantee that the Count will ever go back to zero.
I think we need a new approach.
Follow up for mongodb#388.
I think we should try to reset the buffer automatically, and not expose this to a user.