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

Fix #135 - Support for external io_service #136

Open
wants to merge 1 commit into
base: master
from

Conversation

@nlyan
Copy link

nlyan commented Jul 4, 2017

No description provided.

@coveralls
Copy link

coveralls commented Jul 4, 2017

Coverage Status

Coverage decreased (-1.4%) to 91.86% when pulling a075052 on nlyan:master into 3c0a23d on rpclib:master.

@sztomi sztomi added the ready label Jul 6, 2017
@sztomi sztomi force-pushed the rpclib:master branch from cdf96a5 to 9885c01 Jul 15, 2017
@arximboldi
Copy link

arximboldi commented Sep 29, 2017

Hi! Thanks for the PR, I am interested in this a lot too! But I believe that passing a shared_ptr<> is not a good idea (in my code for example, the io_service is always held in the stack and not via a shared_ptr). Taking a reference must be enough and the lifetime should be controlled by the caller.

@sztomi
Copy link
Collaborator

sztomi commented Sep 29, 2017

Currently there are two obstacles before this is mergable.

  1. rpclib hides its asio dependency. Merging the PR would mean that rpclib now depends on asio on its interface, which is not ideal.
  2. rpclib ships its own asio dependency. You can't easily pass a boost/nonboost asio io_service to rpclib (the versions may mismatch and can affect binary compatibility).

Since this seems fairly popular (and I see the validity of the use case), I'll absolutely make an effort to find a good solution to this after 3.0 is done.

@arximboldi
Copy link

arximboldi commented Sep 29, 2017

@sztomi I see that I noticed that and is also something that I would love the library to stop doing. Same with Msgpack. I understand that this hiding dependencies is maybe useful for Windows users? But maybe it should be optional, because for people using package managers or using these dependencies directly it creates extra trouble (also incompatibilities, duplicate code, versioning/linking problems, etc.)

@sztomi
Copy link
Collaborator

sztomi commented Sep 30, 2017

The linking problems are taken care of (the symbols are renamed), and you can opt out of using the shipped asio (that's why there is a RPCLIB_ASIO define). Msgpack has a few patches added.

The idea is to be completely frictionless, you clone the repo, compile and you are ready to go. A dependency installed from a system package manager is only half of the solution. End users have to go huntig for a compatible version of the dependencies.

I can't argue with duplicate code.

@arximboldi
Copy link

arximboldi commented Sep 30, 2017

@sztomi I agree that embedding the dependencies has advantages for some projects, my point was just that it would be nice to have this be optional since it create problems in others (the issue with not being able to pass your io_service to the library being already an important one). I understand that you might have other priorities though. This is great work anyways, thanks for it!

@sztomi
Copy link
Collaborator

sztomi commented Sep 30, 2017

I'll make sure to have an easy workflow for opting out, at least from asio. It's not terribly hard though, see the documentation here: http://rpclib.net/internals/#dependencies-of-rpclib

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.