mitigate .to_json segfaults on Ruby 1.8.7 #205
Conversation
| when Fixnum, Float, TrueClass, FalseClass, NilClass | ||
| return obj.to_json | ||
| when NilClass | ||
| return "null" |
There was a problem hiding this comment.
Uh, are you sure you want to literally return the string "null"? This sounds like a bad idea.
There was a problem hiding this comment.
Yea I'd agree, returning the String value for a NilClass seems like a bad idea
There was a problem hiding this comment.
Why?
$ ruby -rjson -e 'puts nil.to_json'
null
This is what .to_json gives back on nil-
There was a problem hiding this comment.
Well, yes when you print it, it becomes a string, yes.
I understand that this function actually generates "content" for puppet, so maybe returning the string is ok.
Ug, I just don't like this function :(
Don't forget to adjust the "non"-pretty function as well.
Please add a test for this, I don't want to see this regress.
Something like:
it 'handles null correctly' do
expect(subject.call({"foo" => null}).to eql('"foo"=>null')
end
There was a problem hiding this comment.
I'd think that generating content having it return the value "null" becomes more problematic IMO. I'd personally like if something is actually null the generated content is empty with a null value.
There was a problem hiding this comment.
Well, but that's not how the current implementation works and also how json is specified.
Remember JSON is JavaScript Object Notation and in JavaScript nil is null.
I'm fine with adding the tests that represent/spec the current (pre-this-patch) behavior and hence I will show that the patch does not change it's current behvaior. I will also try to enable travis tests for 1.8.7, so that we could still support this ancient plattform. Yes, I'm very sad to not yet have had the option to move to puppet-server and hence jruby, BUT given that I will need this patch to have the current platform still working properly I thought it makes sense to give such things upstream, as others might run into the same problem on EL6.
Please let me know whether I should do the tests or you are generally not interested in merging that.
There was a problem hiding this comment.
We just need to ensure that it is returning actual null, not the string "null" as those are in fact 2 different things in the JSON spec.
There was a problem hiding this comment.
I agree, but then I would need to return "'null'" (quotes in the string), but we don't want that. .to_json returns a string that is then put into the template. It's confusing, we have layers of strings here...
|
Keep in mind that we do not officially support old versions of ruby in this module (although it isn't super clear from the readme) If this is a real issue for you, I encourage you to submit a PR that re-expands the scope of the .travis.yaml to include your use case. |
|
If I remember correctly the last time we added |
duritong
force-pushed
the
fix_segfault
branch
from
d531913 to
e77e230
Compare
|
So I added:
If you think that anything is missing, please let me know. |
… generate methods.
Regularly we had processes segfaulting on line 13 of consul_sorted_json.rb and we then even had ghost processes (not tracked by passenger anymore) consuming 100% of memory doing nothing when attaching with strace. This might be a problem of either Ruby 1.8.7 itself (similar to https://groups.google.com/forum/#!msg/puppet-users/ag1rAF7o_Uw/1hjtABrmzwgJ) or the used c json library. Looking at what we are doing on line 13, we figured out, that we could reduce the calls to .to_json and hence might be able to mitigate the segfaults. Only changing line 13 helped but moved the segfaults to line 10, looking at that line, we can actually avoid further .to_json calls. Running this patch on 2 puppet masters for 8 hours didn't show any segfaults, nor high running cpu processes, which usually started to appear quite quickly.
duritong
force-pushed
the
fix_segfault
branch
from
e77e230 to
1208066
Compare
|
Well. This looks pretty solid! Thanks! |
mitigate .to_json segfaults on Ruby 1.8.7
Regularly we had processes on our puppetmaster - (still) running on
Ruby 1.8.7 - segfaulting on line 13 of consul_sorted_json.rb and we
then even had ghost processes (not tracked by passenger anymore)
consuming 100% of memory doing nothing when attaching with strace.
This might be a problem of either Ruby 1.8.7 itself (similar to
https://groups.google.com/forum/#!msg/puppet-users/ag1rAF7o_Uw/1hjtABrmzwgJ)
or the used C json library. Looking at what we are doing on line 13,
we figured out, that we could reduce the calls to .to_json and
hence might be able to mitigate the segfaults.
Only changing line 13 helped, but moved the segfaults to line 10,
looking at that line, we figured out, that we can avoid even
further .to_json calls.
Running this patch on 2 puppet masters for 8 hours didn't show any
segfaults, nor any high running cpu processes, which usually started
to appear quite quickly.