Skip to content
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

return event subscribe/unsubscribe errors to local connector users #994

Merged
merged 6 commits into from Feb 27, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Feb 25, 2017

This PR fixes a bug in the local connector that suppressed errors from the subscribe/unsubscribe methods. This uncovered bugs in flux-event and a python test which were fixed.

connector/local: fix error logic in sub/unsub handler
Problem: the local connector method for event
subscribe/unsubscribe was coded so that an error response from
the broker would not be propagated to the API caller.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 25, 2017

Codecov Report

Merging #994 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
+ Coverage   75.85%   75.86%   +0.01%     
==========================================
  Files         152      152              
  Lines       25920    25923       +3     
==========================================
+ Hits        19661    19667       +6     
+ Misses       6259     6256       -3
Impacted Files Coverage Δ
src/modules/connector-local/local.c 71.69% <100%> (-0.12%)
src/connectors/local/local.c 88.04% <100%> (ø)
src/cmd/flux-event.c 65.55% <100%> (+0.38%)
src/common/libflux/tagpool.c 95.12% <0%> (-1.22%)
src/common/libflux/rpc.c 90.22% <0%> (-0.76%)
src/common/libflux/message.c 83.7% <0%> (-0.14%)
src/bindings/lua/flux-lua.c 81.47% <0%> (+0.36%)
src/common/libflux/handle.c 86.73% <0%> (+1.06%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ff2115...325780f. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 25, 2017

Coverage Status

Coverage decreased (-0.005%) to 76.112% when pulling f0a7f05 on garlick:event_error into 8ff2115 on flux-framework:master.

garlick added some commits Feb 22, 2017

modules/connector-local: return ENOENT on unsub fail
Problem: an unsubscribe request that includes a topic
that doesn't match any subscriptions gets an error response
with errnum uninitialized.

Set errnum to ENOENT in the response.
cmd/flux-event: fix unsubscribe logic
Problem: flux-event --count unsubscribes from events when it exits
its event receive loop.  If topic strings are specified on the
command line, the command line is dereferenced incorrectly.

Use the same logic to dereference the command line for unsubscribes
as was used for subscribes.

Note: this problem was previously masked by the lack of error
propagation in the local connector's unsubscribe method.
python/test: fix event unsubscribe test
Problem: python binding's event unsubscribe test attempted
to unsubscribe from a topic string that had not previously
been subscribed to.

In the test, subscribe to the topic before unsubscribing to it.

Note: this problem was previously masked by the lack of error
propagation in the local connector's unsubscribe method.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 26, 2017

Don't merge - on second thought, that unsubscribe failure should be an ENOENT.
I'm adding a test to t/lua/t0003-event.t also.

@garlick garlick force-pushed the garlick:event_error branch from f0a7f05 to 9d0274a Feb 26, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 26, 2017

Coverage Status

Coverage increased (+0.003%) to 76.12% when pulling 9d0274a on garlick:event_error into 8ff2115 on flux-framework:master.

@garlick garlick force-pushed the garlick:event_error branch from 9d0274a to 031045d Feb 26, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 26, 2017

I added a lua test but I encountered what I thought was a bug in the lua bindings where integer C functions would return (nil, "error string") on error rather than (-1, "error string"). Some of the tests around the one I added were checking that the rc was not equal to -1, so I assumed l_pushresult() was not handling this properly and "fixed" it. Then some other tests started failing which were testing rc not equal to nil. I "fixed" those too, but now I'm unsure which way is right or intended.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 26, 2017

Coverage Status

Coverage increased (+0.02%) to 76.14% when pulling 031045d on garlick:event_error into 8ff2115 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 27, 2017

I added a lua test but I encountered what I thought was a bug in the lua bindings where integer C functions would return (nil, "error string") on error rather than (-1, "error string")

Sorry about that! Lua idiom is to return nil,string on error, but I must have been sloppy somewhere in the bindings. Of course, if nil is an expected return value, then some other return code would have to be used to indicate error, but most likely I was just sloppy in the implementation.

I apologize that this caused you grief. Let me take a look and see what the appropriate approach should be.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 27, 2017

Ah, if that's the case then the bindings are correct and there's just a problem with a couple of tests. I'll change the PR to fix the tests not the bindings.

garlick added some commits Feb 26, 2017

test/events: check for nil return code on error
Problem: some event tests check that functions return != -1,
but the lua bindings return nil on error.

Change tests to check for return != nil.

@garlick garlick force-pushed the garlick:event_error branch from 031045d to 325780f Feb 27, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage increased (+0.01%) to 76.131% when pulling 325780f on garlick:event_error into 8ff2115 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 27, 2017

Thanks! Is this ready to merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Feb 27, 2017

Yes please.

@grondo grondo merged commit 1a1ab10 into flux-framework:master Feb 27, 2017

4 checks passed

codecov/patch 100% of diff hit (target 75.85%)
Details
codecov/project 75.86% (+0.01%) compared to 8ff2115
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 76.131%
Details

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

@garlick garlick deleted the garlick:event_error branch Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.