Fix Marshal.dump(RBTree.new)
#2
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyrylo
force-pushed
the
marshal-dump-fix
branch
from
40165c9
to
aa8fab6
Compare
I hit a bug with `Marshal.dump(SortedSet.new)` when `rbtree` is installed. SortedSet uses RBTree when it's present: https://github.com/ruby/ruby/blob/3a083985a471ca3d8429146f9f18dead6747c203/lib/set.rb#L710-L714 Because of that, any class that encapsulates a SortedSet is affected. The bug happens because `rb_marshal_dump` doesn't accept `limit` as its second argument: https://github.com/ruby/ruby/blob/3a083985a471ca3d8429146f9f18dead6747c203/marshal.c#L2272 I'm no C Ruby API expert and I am not sure what `port` means but this change does fix a failing test in this project.
kyrylo
force-pushed
the
marshal-dump-fix
branch
from
aa8fab6
to
99f9e2d
Compare
kyrylo
added a commit
to kyrylo/rbtree3
that referenced
this pull request
Feb 7, 2019
I hit a bug with `Marshal.dump(SortedSet.new)` when `rbtree` is installed. SortedSet uses RBTree when it's present: https://github.com/ruby/ruby/blob/3a083985a471ca3d8429146f9f18dead6747c203/lib/set.rb#L710-L714 Because of that, any class that encapsulates a SortedSet is affected. The bug happens because `rb_marshal_dump` doesn't accept `limit` as its second argument: https://github.com/ruby/ruby/blob/3a083985a471ca3d8429146f9f18dead6747c203/marshal.c#L2272 Ported from skade/rbtree#2
|
I ended up forking this repo and releasing a new version of the gem (gem name is |
kyrylo
added a commit
to airbrake/airbrake-ruby
that referenced
this pull request
Feb 7, 2019
A trail of events lead to this decision: 1. TDigest depends on unmaintained rbtree, which has a bug that our users complain about. An attempt to fix it was made but since the library is unmaintained, nobody merged it: skade/rbtree#2 2. We had to monkey-patch the TDigest library, so our backend understands tdigests. Since we import the library, we no longer need to monkey-patch anything: we can just fix the code itself to make it digestible for us. 3. The TDigest library itself is unmaintained. An attempt to reach original authors revaled that they're busy with other tasks: castle/tdigest#6 TDigests are critical for our performance feature and we cannot afford neglecting the bug that haunts us in [1]. Luckily, the library is tiny (can fit into one file), and its licence (MIT) allows us to modify this code. Of course, to pay respect to the original author, a comment with a link is included into the comments 4. Thanks to all of this, I had a chance to fork rbtree and release it as rbtree3 (https://rubygems.org/gems/rbtree3). This RBTree is patched and should have no know bugs. Since we maintain TDigest now, we can directly depend on it (the TDigest gem depends on bugged rbtree) 5. Not the most important item in the list, but less dependencies means less headaches. That said, I would still perfer not to import anything but we need to move forward and unmaintained libraries slow us down
kyrylo
added a commit
to airbrake/airbrake-ruby
that referenced
this pull request
Feb 7, 2019
A trail of events lead to this decision: 1. TDigest depends on unmaintained rbtree, which has a bug that our users complain about. An attempt to fix it was made but since the library is unmaintained, nobody merged it: skade/rbtree#2 2. We had to monkey-patch the TDigest library, so our backend understands tdigests. Since we import the library, we no longer need to monkey-patch anything: we can just fix the code itself to make it digestible for us. 3. The TDigest library itself is unmaintained. An attempt to reach original authors revaled that they're busy with other tasks: castle/tdigest#6 TDigests are critical for our performance feature and we cannot afford neglecting the bug that haunts us in [1]. Luckily, the library is tiny (can fit into one file), and its licence (MIT) allows us to modify this code. Of course, to pay respect to the original author, a comment with a link is included into the comments 4. Thanks to all of this, I had a chance to fork rbtree and release it as rbtree3 (https://rubygems.org/gems/rbtree3). This RBTree is patched and should have no know bugs. Since we maintain TDigest now, we can directly depend on it (the TDigest gem depends on bugged rbtree) 5. Not the most important item in the list, but less dependencies means less headaches. That said, I would still perfer not to import anything but we need to move forward and unmaintained libraries slow us down
kyrylo
added a commit
to airbrake/airbrake-ruby
that referenced
this pull request
Feb 7, 2019
A trail of events lead to this decision: 1. TDigest depends on unmaintained rbtree, which has a bug that our users complain about. An attempt to fix it was made but since the library is unmaintained, nobody merged it: skade/rbtree#2 2. We had to monkey-patch the TDigest library, so our backend understands tdigests. Since we import the library, we no longer need to monkey-patch anything: we can just fix the code itself to make it digestible for us. 3. The TDigest library itself is unmaintained. An attempt to reach original authors revaled that they're busy with other tasks: castle/tdigest#6 TDigests are critical for our performance feature and we cannot afford neglecting the bug that haunts us in [1]. Luckily, the library is tiny (can fit into one file), and its licence (MIT) allows us to modify this code. Of course, to pay respect to the original author, a comment with a link is included into the comments 4. Thanks to all of this, I had a chance to fork rbtree and release it as rbtree3 (https://rubygems.org/gems/rbtree3). This RBTree is patched and should have no know bugs. Since we maintain TDigest now, we can directly depend on it (the TDigest gem depends on bugged rbtree) 5. Not the most important item in the list, but less dependencies means less headaches. That said, I would still perfer not to import anything but we need to move forward and unmaintained libraries slow us down
|
@kyrylo Hi, sorry for the late reply. This is probably the best option, as this is not the authoritative repos and I have no access to the gem on rubygems.org. Sorry for missing this patch :). |
|
No worries and thanks for the reply 👍 |
nevans
pushed a commit
to nevans/rbtree
that referenced
this pull request
Nov 5, 2020
circle: make it run properly
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hit a bug with
Marshal.dump(SortedSet.new)whenrbtreeis installed.SortedSet uses RBTree when it's present:
https://github.com/ruby/ruby/blob/3a083985a471ca3d8429146f9f18dead6747c203/lib/set.rb#L710-L714
Because of that, any class that encapsulates a SortedSet is affected.
The bug happens because
rb_marshal_dumpdoesn't acceptlimitas its secondargument:
https://github.com/ruby/ruby/blob/3a083985a471ca3d8429146f9f18dead6747c203/marshal.c#L2272
I'm no C Ruby API expert and I am not sure what
portmeans but this changedoes fix a failing test in this project.