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

On call issue 239 fix #245

Merged
merged 3 commits into from
Dec 18, 2017
Merged

On call issue 239 fix #245

merged 3 commits into from
Dec 18, 2017

Conversation

seveirein
Copy link
Contributor

@seveirein seveirein commented Dec 14, 2017

Issue #239 fix here. It could use some documentation being added, but thought I'd put it for debate first. This basically fixes it by checking the root service for an option "on_call" method, and if available, that method is used to dispatch the calls instead.

This is also one of 3 pull requests (PR #245, PR #246, PR #247) I am submitting that I need acceptance on to go forward with a large optional 3rd party support library for RPyC that adds allows for an enhanced security model.

@coldfix
Copy link
Contributor

coldfix commented Dec 14, 2017

I'm open for a similar patch, with few changes:

Rename Connection_do_call_by_obj to _dispatch_call, same for Service.on_call.

Resolve to Service._dispatch_call at Connection.__init__ time by if hasattr(root, '_dispatch_call'): self._dispatch_call = root._dispatch_call (so there will be no if inside Conection._dispatch_call).

@seveirein
Copy link
Contributor Author

I'm fine with renaming the call to service to ._dispatch_call. Renaming any function "_dispatch*" in protocol.py however will break it, because it scans for functions named that at the end of it and matches them with protocol DISPATCH constants.

@seveirein
Copy link
Contributor Author

Oops, sorry, confused _handle with _dispatch. Working on patch.

@seveirein
Copy link
Contributor Author

Okay, changes made. Check to see that you are happy with how the dict() call on kwargs ended up getting moved. I'd prefer not to have the keyword arguments passed to me in the tuple format. There is no preserving ordering for them anyways, as they come from kwargs stored in a dictionary.

Also, how would you like this documented? I think we should get this documented in Sphinx before this pull request is merged in finally.

@coldfix
Copy link
Contributor

coldfix commented Dec 16, 2017

You don't seem to have pushed your changes.

@seveirein
Copy link
Contributor Author

Hmm, guess I committed and didn't push. That means I had to redo the changes since I won't have access to the machine I did that on for almost a month (away on vacation). I have redone the changes/tested them/and pushed them.

@coldfix coldfix merged commit 5e97e3f into tomerfiliba-org:master Dec 18, 2017
@coldfix
Copy link
Contributor

coldfix commented Dec 18, 2017

Thanks

coldfix added a commit that referenced this pull request Dec 21, 2017
This reverts PR #245 (on issue #239).

I finally decided it's much cleaner to go for a more generic approach
where the service can easily install itself into

    conn._HANDLERS[HANDLE_CALL]

Getting a special treatment for function calls feels kind of awkward and
is unnecessary…
coldfix added a commit that referenced this pull request Dec 21, 2017
This reverts PR #245 (on issue #239).

I finally decided it's much cleaner to go for a more generic approach
where the service can easily install itself into

    conn._HANDLERS[HANDLE_CALL]

Getting a special treatment for function calls feels kind of awkward and
is unnecessary…
coldfix added a commit that referenced this pull request Dec 21, 2017
This reverts PR #245 (on issue #239).

I finally decided it's much cleaner to go for a more generic approach
where the service can easily install itself into

    conn._HANDLERS[HANDLE_CALL]

Getting a special treatment for function calls feels kind of awkward and
is unnecessary…
@coldfix
Copy link
Contributor

coldfix commented Dec 21, 2017

Hi,

I have changed my mind about this and reverted these changes. But fear not: you will now have an alternative solution that is even more powerful and generic (all the while requiring less code and keeping it simpler):

The service object now controls which protocol to use. All you need to do is set the _protocol attribute for your service. Therefore, you can more easily subclass the protocol (i.e. Connection) and override _handle_call or any of the other handlers. One obvious advantage is that _handle_call or _unbox_exception are no longer ruled out as special cases. You can override e.g. _handle_getattr the very same way.

Alternatively, the request handlers can individually be overriden by installing a custom handler into conn._HANDLERS, e.g.:

class Service(rpyc.Service):
  def on_connect(self, conn):
    conn._HANDLERS.update({
      HANDLE_CALL: self._handle_call,
    })
  def _handle_call(self, conn, obj, args, kwargs):
      print("dispatching call:", obj, args, kwargs)
      return conn._handle_call(obj, args, kwargs)

Note that the request handlers now feel much more high-level because they receive the object itself rather than the object ID. This makes them a lot more convenient to override.

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