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

drop coprocess programming model #1081

Merged
merged 14 commits into from May 28, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented May 28, 2017

Since I think we're now "all in" with the future/continuation model of concurrency in reactor programming, and the coprocess handle mode was only used in tests (and in a convoluted way at that), this PR reworks those tests and drops coprocess support.

The immediate benefit is that the message dispatch code is simplified. But the reworked tests are now also more serviceable.

The RPC tests, which are meant to be unit tests run before the the broker is tested, used a loop connector and the FLUX_O_COPROC flag to simulate a server thread. The replacement method is a simple modification to the shmem connector that allows two flux_t handles to be set up back-to-back. Instead of running rpcs and service methods in the same thread, the tests now run service methods in a separate thread. The code for setting up the test, which is reminiscent of czmq's zactor class, is abstracted in t/rpc/util.[ch], and was not dignified with a place in the Flux API since back to back flux_t handles is a pretty weird construct.

While removing the specialized coproc dispatcher, I got rid of the deferred message handler deletion code that @grondo recently noted was unnecessary since zlist_remove is safe during a list walk.

Finally, I hit bug #1063 a few times running make check on cloud9 so I included a fix for that.

garlick added some commits May 26, 2017

connectors/shmem: add bind capability
Add bind capability to the shmem connector.

If user calls flux_open ("shmem://uuid&bind") then the socket
is bound instead of connected.  This allows two shmem connectors
to be used back to back in testing.

For symmetry also parse "shmem://uuid&connect".  This is the default
if no options are provided.
t/shmem: add test for shmem://
Connect two handles back to back using shmem://test&bind
and shmem://test&connect and exchange messages.
t/rpc: rework rpc test to use shmem://
Rework the rpc test to use back to back shmem://
connectors rather than the loop:// connector and
coproc mode.  This test set up is much easier to
understand and maintain going forward.
connectors/shmem: implement setopt for testing
Implement the FLUX_OPT_TESTING_ROLEMASK and _USERID
connector options like the local connector.

This allows messages that have uninitialized rolemask
and/or userid to be set to test values by setting
environment variables FLUX_HANDLE_ROLEMASK or
FLUX_HANDLE_USERID.
connectors/shmem: send should not modify route stack
op_send should not push identity onto route stack for
events and requests.  For back-to-back testing it is
not desirable.

Update broker module_recvmsg() to do this instead.
t/rpc: rework mrpc test to use shmem://
Rework the mrpc test to use back to back shmem://
connectors rather than the loop:// connector and
coproc mode.  This test set up is much easier to
understand and maintain going forward.
broker: register content attributes early
Occasionally this sharness test fails:

not ok 5 - Attempt to start instance with invalid hash fails hard
FAIL: t2008-althash.t 5 - Attempt to start instance with invalid hash fails hard
 #
 #  test_must_fail flux start -o,-Scontent.hash=wronghash /bin/true
 #
The broker asserts in zsys teardown:
 src/zsock_option.c:1347: zsock_set_sndtimeo:
   Assertion `rc == 0 || zmq_errno () == (156384712 + 53)' failed.

Since we are deliberately triggering this early failure in sharness,
move the content-cache attribute initialization to before
zsys is initialized.

Fixes #1063
t/request: drop coproc test
Drop portion of request test that checks COPROC mode.
libflux/dispatch: drop FLUX_O_COPROC flag
Drop the FLUX_O_COPROC flag, the flux_sleep_on() API
call,  and the code in the message dispatcher that handles
specialized coproc mode dispatch.

This mode was not used outside of tests, so in the
interest of de-complicating the dispatcher, drop it.
t/loop/reactor: drop coproc reference
Drop test that calls flux_sleep_on() and verifies that it's
an EINVAL outside of coproc context.
libutil/coproc: drop unused class
Drop unused coproc class and its unit test.
doc/flux_recv(3): drop coproc docs
Drop explanation of coprocess mode.

Also drop FLUX_O_COPROC from flux_open(3).
libflux/dispatch: don't defer handler deletion
Since it's safe to call zlist_remove() while walking a zlist,
drop the 'destroyed' flag and associated deferred-delete handling
code, and just zlist_remove() the message handler from the
dispatcher lists in flux_msg_handler_destroy ().
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 28, 2017

Coverage Status

Coverage increased (+0.09%) to 78.279% when pulling 1e9d058 on garlick:shmem_bind into 655adad on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 28, 2017

Codecov Report

Merging #1081 into master will increase coverage by 0.08%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1081      +/-   ##
==========================================
+ Coverage   77.94%   78.03%   +0.08%     
==========================================
  Files         151      150       -1     
  Lines       26123    25954     -169     
==========================================
- Hits        20362    20252     -110     
+ Misses       5761     5702      -59
Impacted Files Coverage Δ
src/common/libflux/handle.c 85.38% <ø> (+0.96%) ⬆️
src/broker/broker.c 72.78% <100%> (+0.02%) ⬆️
src/broker/module.c 83.66% <100%> (+1.5%) ⬆️
src/common/libflux/dispatch.c 86.55% <100%> (+2.99%) ⬆️
src/connectors/shmem/shmem.c 83.33% <77.55%> (-2.99%) ⬇️
src/connectors/loop/loop.c 83.65% <0%> (-0.97%) ⬇️
src/common/libflux/message.c 81.33% <0%> (-0.36%) ⬇️
src/modules/connector-local/local.c 70.28% <0%> (-0.21%) ⬇️
src/common/libutil/dirwalk.c 94.07% <0%> (ø) ⬆️
src/bindings/lua/flux-lua.c 81.62% <0%> (+0.09%) ⬆️
... and 10 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 28, 2017

It is too bad to lose the coproc code, which is was very cool and clever. However, this looks like some real nice cleanup! Ready to merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 28, 2017

Yes should be ready.

I felt the same way, but the way we integrated it with the reactor was fairly limited. We can always revisit someday if we want to explore alternate models and do a better job. But for now I think it's best set aside.

@grondo grondo merged commit 17347fd into flux-framework:master May 28, 2017

4 checks passed

codecov/patch 83.33% of diff hit (target 77.94%)
Details
codecov/project 78.03% (+0.08%) compared to 655adad
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 78.279%
Details
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 28, 2017

Nice work on revamping the tests, btw. That model will be handy!

@garlick garlick deleted the garlick:shmem_bind branch May 28, 2017

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.