Use riak_core_capabilities:get_supported/2 to avoid crash #266

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@janpieper

I'm quite new to erlang and riak_core but i think i just found a bug which can cause a crash. Two weeks ago i already created an issue for Sean Cribbs' riak_id (see seancribbs/riak_id#3 for further details) because i thought the bug is located within riak_id. Since i got no response for about two weeks i tried to debug the error and found orddict:fetch(node(), State#state.supported) which will raise an error if node() cannot be found in State#state.supported. It seems like get_supported/2 is already handling that case so i changed orddict:fetch(...) to get_supported(...).

@slfritchie

This comment has been minimized.

Show comment Hide comment
@slfritchie

slfritchie Sep 27, 2013

Contributor

Time for some belated repo curating ... @jrwest @jtuple @jonmeredith I think this addresses #227. If not, it'd be great to have another eye to bless this small PR.

Contributor

slfritchie commented Sep 27, 2013

Time for some belated repo curating ... @jrwest @jtuple @jonmeredith I think this addresses #227. If not, it'd be great to have another eye to bless this small PR.

@jrwest

This comment has been minimized.

Show comment Hide comment
@jrwest

jrwest Sep 27, 2013

Contributor

I'm concerned about silently handling this case. Granted to users the crash is completely uninformative but if the node name changes, renegotiating capabilities may now succeed but the node name has not changed in the ring, etc. Perhaps we should catch the case earlier, instead, so the assumption renegotiate_capabilities makes in its use of orddict:fetch is valid?

EDIT: looks like #243 is what i'm talking about

Contributor

jrwest commented Sep 27, 2013

I'm concerned about silently handling this case. Granted to users the crash is completely uninformative but if the node name changes, renegotiating capabilities may now succeed but the node name has not changed in the ring, etc. Perhaps we should catch the case earlier, instead, so the assumption renegotiate_capabilities makes in its use of orddict:fetch is valid?

EDIT: looks like #243 is what i'm talking about

@slfritchie

This comment has been minimized.

Show comment Hide comment
@slfritchie

slfritchie Sep 28, 2013

Contributor

EDIT: looks like #243 is what i'm talking about

Hrmm, so if #243 is merged (i.e., soon 'cause it's now been positively reviewed), then ... what, sorry?

Contributor

slfritchie commented Sep 28, 2013

EDIT: looks like #243 is what i'm talking about

Hrmm, so if #243 is merged (i.e., soon 'cause it's now been positively reviewed), then ... what, sorry?

@jrwest

This comment has been minimized.

Show comment Hide comment
@jrwest

jrwest Sep 28, 2013

Contributor

@slfritchie you lost me. #243 is an issue not a PR? Was trying to say, if we handle the case that @evanmcc calls a stale ring file (where we would hit the code path this PR is meant to address) then if the node was exited this PR isn't needed.

Contributor

jrwest commented Sep 28, 2013

@slfritchie you lost me. #243 is an issue not a PR? Was trying to say, if we handle the case that @evanmcc calls a stale ring file (where we would hit the code path this PR is meant to address) then if the node was exited this PR isn't needed.

@jrwest

This comment has been minimized.

Show comment Hide comment
@jrwest

jrwest Sep 28, 2013

Contributor

oh wait, i see #244 attached to #243 now. I'm +1 on that approach as long as the node crashes but that PR replaces this one doesn't it?

Contributor

jrwest commented Sep 28, 2013

oh wait, i see #244 attached to #243 now. I'm +1 on that approach as long as the node crashes but that PR replaces this one doesn't it?

@slfritchie

This comment has been minimized.

Show comment Hide comment
@slfritchie

slfritchie Dec 12, 2013

Contributor

Hi, Jan. Many thanks for the bugfix. Evan's later fix in #244 takes care of the same problem while also addressing another problem. Manymany apologies for the delay! Closing.

Contributor

slfritchie commented Dec 12, 2013

Hi, Jan. Many thanks for the bugfix. Evan's later fix in #244 takes care of the same problem while also addressing another problem. Manymany apologies for the delay! Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment