Skip to content

Fix mango native proc crash#1067

Merged
eiri merged 3 commits intoapache:masterfrom
cloudant:fix-mango_native_proc-crash
Dec 20, 2017
Merged

Fix mango native proc crash#1067
eiri merged 3 commits intoapache:masterfrom
cloudant:fix-mango_native_proc-crash

Conversation

@eiri
Copy link
Copy Markdown
Member

@eiri eiri commented Dec 14, 2017

Overview

When soft limit is reached couch_proc_manager evicts idle processes by casting on them stop
message. Since mango_native_proc doesn't handle this message it results to its crash with
invalid_cast reason.

Testing recommendations

make eunit apps=mango or make eunit apps=mango suites=mango_native_proc for isolated test.

make mango-test for integration test.

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

eiri added 2 commits December 14, 2017 17:29
When soft limit is reached couch_proc_manager
evicts idle processes by casting on them `stop`
message. Since mango_native_proc doesn't handle
this message it results to its crash with
`invalid_cast` reason.
erlang:yield(),
?assertEqual({noreply, []}, handle_cast(garbage_collect, [])),
receive
{'DOWN', TracerRef, _, _, Msg} -> ?assertEqual(gc_start, Msg)
Copy link
Copy Markdown
Contributor

@tonysun83 tonysun83 Dec 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So iiuc, when the spawned process starts garbage collection, it'll exit with gc_start which would then cause our mango_native_proc to exit with an {invalid_cast, gc_start}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no, it's different.

We spawn and monitor a new process that sets a trace on the test process, because process can't trace itself. Then in the test process we call a garbage collection by executing handle_cast(garbage_collect, []) and asserting that we are getting proper noreply response. The tracer receives gc_start trace message, indicating that its target (the test process) started a garbage collection and exits with reason gc_start. Because we are monitoring it in our test process, we are receiving its exit reason and can assert that it was indeed for a garbage collection.

Interesting bit here is erlang:yield() - we need to give a scheduler a chance to run the tracer process or gc will be finished before the trace set.

Conceptually it's is not different from when we are mocking module with meck and then waiting for some of its functions to be called, but I had to use trace here because you can't mock garbage_collection. Plus, unlike mock, this is a real deal, we are checking actual, not simulated functionality.

Copy link
Copy Markdown
Contributor

@tonysun83 tonysun83 Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eiri : I completely forgot to check that we have handle_cast(garbage_collect, St) in the source. That's what confused me. This test was for that functionality. Got it!

@tonysun83
Copy link
Copy Markdown
Contributor

The fix lgtm. The garbage collection test is a little tricky, just want to understand it a bit better.

@eiri
Copy link
Copy Markdown
Member Author

eiri commented Dec 19, 2017

Hey @tonysun83 , was my explanation sufficient? Do you want me to change anything in the tests?

@tonysun83
Copy link
Copy Markdown
Contributor

+1

@eiri eiri merged commit 3b28b84 into apache:master Dec 20, 2017
@eiri eiri deleted the fix-mango_native_proc-crash branch December 20, 2017 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants