-
Notifications
You must be signed in to change notification settings - Fork 0
CSHARP-1525: JsonBuffer can exhaust memory. #4
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,8 @@ namespace MongoDB.Bson.IO | |
| internal class JsonBuffer | ||
| { | ||
| // private fields | ||
| private readonly int _readChunkSize; | ||
| private readonly int _minDiscardBufferSize; | ||
| private readonly StringBuilder _buffer; | ||
| private int _position; | ||
| private readonly TextReader _reader; | ||
|
|
@@ -43,18 +45,22 @@ public JsonBuffer(string json) | |
| _buffer = new StringBuilder(json); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="JsonBuffer" /> class. | ||
| /// </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> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missed params |
||
| /// <param name="minResetBufferSize"></param> | ||
| public JsonBuffer(TextReader reader, int readChunkSize, int minResetBufferSize) | ||
| { | ||
| if (reader == null) | ||
| { | ||
| throw new ArgumentNullException("reader"); | ||
| } | ||
| _buffer = new StringBuilder(256); // start out with a reasonable initial capacity | ||
| _reader = reader; | ||
|
|
||
| // TODO BD validation | ||
| _readChunkSize = readChunkSize; | ||
| _minDiscardBufferSize = minResetBufferSize; | ||
| } | ||
|
|
||
| // public properties | ||
|
|
@@ -146,8 +152,7 @@ public int Read() | |
| public void ResetBuffer() | ||
| { | ||
| // only trim the buffer if enough space will be reclaimed to make it worthwhile | ||
| var minimumTrimCount = 256; // TODO: make configurable? | ||
| if (_position >= minimumTrimCount) | ||
| if (_position >= _minDiscardBufferSize) | ||
| { | ||
| _buffer.Remove(0, _position); | ||
| _position = 0; | ||
|
|
@@ -189,9 +194,8 @@ private void ReadMoreIfAtEndOfBuffer() | |
| { | ||
| if (_reader != null) | ||
| { | ||
| var blockSize = 1024; // TODO: make configurable? | ||
| var block = new char[blockSize]; | ||
| var actualCount = _reader.ReadBlock(block, 0, blockSize); | ||
| var block = new char[_readChunkSize]; | ||
| var actualCount = _reader.ReadBlock(block, 0, _readChunkSize); | ||
|
|
||
| if (actualCount > 0) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,6 +72,8 @@ public class JsonReader : BsonReader | |
| private BsonValue _currentValue; | ||
| private JsonToken _pushedToken; | ||
|
|
||
| private readonly List<JsonReaderBookmark> _bookmarks = new List<JsonReaderBookmark>(); | ||
|
|
||
| // constructors | ||
| /// <summary> | ||
| /// Initializes a new instance of the JsonReader class. | ||
|
|
@@ -107,7 +109,7 @@ public JsonReader(TextReader textReader) | |
| /// <param name="textReader">The TextReader.</param> | ||
| /// <param name="settings">The reader settings.</param> | ||
| public JsonReader(TextReader textReader, JsonReaderSettings settings) | ||
| : this(new JsonBuffer(textReader), settings) | ||
| : this(new JsonBuffer(textReader, settings.ReadChunkSize, settings.MinResetBufferSize), settings) | ||
| { | ||
| } | ||
|
|
||
|
|
@@ -135,7 +137,10 @@ public override void Close() | |
| /// <returns>A bookmark.</returns> | ||
| public override BsonReaderBookmark GetBookmark() | ||
| { | ||
| return new JsonReaderBookmark(State, CurrentBsonType, CurrentName, _context, _currentToken, _currentValue, _pushedToken, _buffer.Position); | ||
| var bookmark = new JsonReaderBookmark(State, CurrentBsonType, CurrentName, _context, _currentToken, _currentValue, _pushedToken, _buffer.Position); | ||
| _bookmarks.Add(bookmark); | ||
|
|
||
| return bookmark; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -399,6 +404,12 @@ public override BsonType ReadBsonType() | |
| State = BsonReaderState.Value; | ||
| break; | ||
| } | ||
|
|
||
| if (_jsonReaderSettings.AutoBufferReset && _bookmarks.Count == 0) | ||
| { | ||
| _buffer.ResetBuffer(); | ||
| } | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try to reset after reading a token (buffer will reset only if
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no requirement that Therefore, there is no guarantee that the I think we need a new approach. |
||
|
|
||
| return CurrentBsonType; | ||
| } | ||
|
|
||
|
|
@@ -749,6 +760,7 @@ public override void ReadUndefined() | |
| public override void ReturnToBookmark(BsonReaderBookmark bookmark) | ||
| { | ||
| if (Disposed) { ThrowObjectDisposedException(); } | ||
|
|
||
| var jsonReaderBookmark = (JsonReaderBookmark)bookmark; | ||
| State = jsonReaderBookmark.State; | ||
| CurrentBsonType = jsonReaderBookmark.CurrentBsonType; | ||
|
|
@@ -758,6 +770,9 @@ public override void ReturnToBookmark(BsonReaderBookmark bookmark) | |
| _currentValue = jsonReaderBookmark.CurrentValue; | ||
| _pushedToken = jsonReaderBookmark.PushedToken; | ||
| _buffer.Position = jsonReaderBookmark.Position; | ||
|
|
||
| // TODO BD throw if not found? | ||
| _bookmarks.Remove(jsonReaderBookmark); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Show we disallow custom created bookmarks (maybe as an |
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,21 @@ public class JsonReaderSettings : BsonReaderSettings | |
| // private static fields | ||
| private static JsonReaderSettings __defaults = null; // delay creation to pick up the latest default values | ||
|
|
||
| /// <summary> | ||
| /// TODO BD | ||
| /// </summary> | ||
| public bool AutoBufferReset { get; set; } = true; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the current styling guide, we don't use auto properties
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, properties should be placed after constructor |
||
|
|
||
| /// <summary> | ||
| /// TODO BD | ||
| /// </summary> | ||
| public int ReadChunkSize { get; set; } = 2048; | ||
|
|
||
| /// <summary> | ||
| /// TODO BD | ||
| /// </summary> | ||
| public int MinResetBufferSize { get; set; } = 512; | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use an approach that doesn't involve adding any new public APIs?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can, but:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a "no knobs" principle at MongoDB. In practice that really means "as few knobs as possible".
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, would we want to leave JsonReaderSettings.AutoBufferReset if we were to proceed with this approach?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // constructors | ||
| /// <summary> | ||
| /// Initializes a new instance of the JsonReaderSettings class. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use the default formatting for xml comment: