bpo-29615: SimpleXMLRPCDispatcher no longer chains KeyError (or any other exception) to exception(s) raised in the dispatched methods. #260

petr-motejlek · 2017-02-23T19:52:10Z

No description provided.

the-knights-who-say-ni · 2017-02-23T19:52:11Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

serhiy-storchaka · 2017-02-24T08:09:10Z

Lib/xmlrpc/server.py

-        else:
-            raise Exception('method "%s" is not supported' % method)
+                def func(*params):
+                    return dispatch_func(method=method, params=params)


It is not required that the dispatch function supports keyword arguments.

What is the purpose of this change? Why not call the dispatch function directly?

return func(method, params)

It is not required that the dispatch function supports keyword arguments.
Agreed, changed.

What is the purpose of this change? Why not call the dispatch function directly?
The purpose of this change is to make sure no exception handling is in progress when the function is called, thus I do attempt to first resolve the function (handling exceptions as I go) and then once the function is (hopefully) resolved, I attempt to call it

I did add @functools.wraps around the wrapper (forgot to check that in earlier)

That is why I suggest to move all code after except KeyError out of exception handling block. This will allow to get rid of exception chaining and may make the code cleaner.

serhiy-storchaka · 2017-02-24T08:09:10Z

Lib/xmlrpc/server.py

+                        self.allow_dotted_names
+                    )
+                except AttributeError:
+                    raise Exception('method "%s" is not supported' % method)


This chains two AttributeErrors and a KeyError to the raised exception.

I know. But is that a problem, though? For me, this was the intended feature as it lets the caller know (using the exception chain) what steps were taken (and failed) to find the appropriate function

That looks a problem for me. These exceptions don't add useful information and are implementation details. For example current implementation uses hasattr() rather than catching an AttributeError.

Let's leave the current implementation aside in terms of what's correct and incorrect :)

However, I will remove the chaining, as you suggest.

Chaining removed

serhiy-storchaka · 2017-02-24T08:09:10Z

Lib/xmlrpc/server.py

+            except AttributeError:
+                # call instance method directly
+                try:
+                    func = resolve_dotted_attribute(


What if resolve_dotted_attribute() returns None?

Hi @serhiy-storchaka , this is my 1st PR and 1st comment, so let me take this opportunity to thank you for the review :).

What about other (non-callable) objects, though? I mean, it's all the same and it doesn't really matter if whatever the function returns is NoneType or, let's say, int(2) --> it's still wrong. The original code does not check that either and it simply attempts to call the result.

I would let Python deal with the NoneType and other non-callable types and raise it's own TypeError and leave this part of the code untouched. It is still a subclass of Exception, so the signature, if you will, of this function is still the same

None is handled special in current code. Existing code can set an attribute to None for disabling an inherited method. A user get a more informative exception 'method "{methodname}" is not supported' rather than "'NoneType' object is not callable". I don't know whether this is used in real code, but there is a risk of breaking existing code.

OK, let me fix that

Fixed, although the code is now a little bit more bloated than I would have liked, but it works.

…y argument call

serhiy-storchaka · 2017-02-27T14:30:21Z

Lib/xmlrpc/server.py

+                        self.allow_dotted_names
+                    )
+                except AttributeError:
+                    raise Exception('method "%s" is not supported' % method)


That looks a problem for me. These exceptions don't add useful information and are implementation details. For example current implementation uses hasattr() rather than catching an AttributeError.

serhiy-storchaka · 2017-02-27T14:30:21Z

Lib/xmlrpc/server.py

+            except AttributeError:
+                # call instance method directly
+                try:
+                    func = resolve_dotted_attribute(


None is handled special in current code. Existing code can set an attribute to None for disabling an inherited method. A user get a more informative exception 'method "{methodname}" is not supported' rather than "'NoneType' object is not callable". I don't know whether this is used in real code, but there is a risk of breaking existing code.

serhiy-storchaka · 2017-02-27T14:30:21Z

Lib/xmlrpc/server.py

-        else:
-            raise Exception('method "%s" is not supported' % method)
+                def func(*params):
+                    return dispatch_func(method=method, params=params)


That is why I suggest to move all code after except KeyError out of exception handling block. This will allow to get rid of exception chaining and may make the code cleaner.

…ts to invoke `None`

serhiy-storchaka

Needed new test. Add an entry in Misc/NEWS.

serhiy-storchaka · 2017-02-27T20:34:46Z

Lib/xmlrpc/server.py

@@ -103,7 +103,7 @@ def export_add(self, x, y):

 # Written by Brian Quinlan (brian@sweetapp.com).
 # Based on code written by Fredrik Lundh.
-
+import functools


serhiy-storchaka · 2017-02-27T20:34:46Z

Lib/test/test_xmlrpc.py

@@ -343,6 +343,24 @@ def run_server():
            self.assertEqual(p.method(), 5)
            self.assertEqual(p.method(), 5)

+    def test_simple_xml_rpc_dispatcher_using_dispatch_func(self):


The test is passed when leave Lib/xmlrpc/server.py unchanged. Thus it doesn't test changes.

My bad. I forgot to check in the proper test earlier.

serhiy-storchaka · 2017-02-27T20:34:46Z

Lib/xmlrpc/server.py

+            except AttributeError:
+                pass
+
+            if func is not None:


For compatibility with current code it should be just else if use try..except.

serhiy-storchaka · 2017-02-27T20:34:46Z

Lib/xmlrpc/server.py

+        if self.instance is not None:
+            try:
+                # check for a _dispatch method on the instance
+                func = self.instance._dispatch


It would be better to use other name for local variable.

But I suggest to keep the current code, with hasattr(). Catching exceptions is costly in Python, and if _dispatch is not defined this causes regression.

Ok. Rewritten. I believe it is now in sync with the original code in every aspect, but the exception chaning issue I am attempting to solve.

…tting rid of the exception chaining

… no longer chained to exceptions raised within `_dispatch` function

petr-motejlek · 2017-02-28T11:57:47Z

Hi @serhiy-storchaka , in addition to the changes you requested directly in the code, I added a line to Misc/NEWS and merged the latest master. I hope we are getting closer to the end :)

EDIT: Travis complained about test coverage, so I added tests that ensure that all 3 methods of locating the function to be dispatched work and neither of them chains any exceptions when the function gets called

In order to get 100 % coverage of the _dispatch function, though, I would also need to test the negative case when no function can be located, however the exception raised in that case is a simple Exception which is very generic. What do you suggest to do about this? Should I ignore the fact that the exception is generic and still try to catch it and make sure that what I caught was an instance of Exception and not a subclass of it? Something else?

Thanks

…ace when calling dispatched funcs

serhiy-storchaka

Yes, I hope the next iteration will be the last or close to end. :-)

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/test/test_xmlrpc.py

+        try:
+            dispatcher._dispatch(method='dispatched_func', params=exp_params)
+        except DispatchExc as e:
+            self.assertFalse(e.__cause__)


assertIsNone()

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/test/test_xmlrpc.py

+        dispatcher = xmlrpc.server.SimpleXMLRPCDispatcher()
+        dispatcher.register_function(dispatched_func)
+        try:
+            dispatcher._dispatch(method='dispatched_func', params=exp_params)


I think it would be better to pass arguments as positional. It is shorter and more reliable. Parameter names is an implementation detail that we don't want to test here.

Sure. I am sorry about this, but in my code I tend to use keywords all the time, it makes it so much easier to understand what the code does when you see the keywords :). Removed.

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/test/test_xmlrpc.py

+        dispatcher.register_function(dispatched_func)
+        try:
+            dispatcher._dispatch(method='dispatched_func', params=exp_params)
+        except DispatchExc as e:


The test is passed if DispatchExc is not raised.

Use assertRaises() as a context manager.

with self.assertRaises(DispatchExc) as cm: dispatcher._dispatch('dispatched_func', exp_params) self.assertIsNone(cm.exception.__cause__)

OH, snap. I always forget that context variables are visible outside the with statement. Sure will do this, that's great.

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/test/test_xmlrpc.py

@@ -343,6 +343,78 @@ def run_server():
            self.assertEqual(p.method(), 5)
            self.assertEqual(p.method(), 5)

+    def test_simple_xml_rpc_dispatcher_resolves_registered_func_and_does_not_chain_excs_when_calling_it(self):
+        exp_params = 1, 2, 3
+        dispatched = False


Checking that dispatched is set is redundant in these tests. It is obvious that it is set since the exception is raised after that. By removing dispatched you can save few lines of code.

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/test/test_xmlrpc.py

+        exp_params = 1, 2, 3
+        dispatched = False
+
+        class DispatchExc(Exception):


DispatchExc is used in several tests. It is worth to define it at the module level.

I hope it's ok to define it at a test case level for now. (SimpleXMLRPCDispatcherTestCase.DispatcherExc that is)

You will need to specify long qualified name (SimpleXMLRPCDispatcherTestCase.DispatcherExc) or use outer self to access it.

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/xmlrpc/server.py

+                raise Exception('method "%s" is not supported' % method)
+            return func(*params)
+
+        if self.instance is None:
            raise Exception('method "%s" is not supported' % method)


raise Exception() is repeated multiple times. I think it is enough to repeat it only two times. Once when self.funcs[method] is None, and other time at the end of this method, as in the previous version of your code.

OK, right. That's true. Done.

serhiy-storchaka · 2017-02-28T16:32:39Z

Lib/xmlrpc/server.py

+        if hasattr(self.instance, '_dispatch'):
+            # call the `_dispatch` method on the instance
+            return self.instance._dispatch(method, params)
+        else:


Since the other branch is ended by return, else is redundant.

serhiy-storchaka · 2017-02-28T16:32:39Z

Misc/NEWS

@@ -10,6 +10,10 @@ What's New in Python 3.7.0 alpha 1?
 Core and Builtins
 -----------------

+- issue29615: xmlrpc.server.SimpleXMLRPCDispatcher.instance._dispatch function is no longer called in an except block


The line is too long. Add periods after ends of sentences. Use "bpo-29615" instead of "issue29615". This entry should be in the "Library" section rather of "Core and Builtins".

I think it would be better to reformulate the entry as "SimpleXMLRPCDispatcher no longer adds KeyError exception in the traceback of exception raised in the method of the registered instance." Or something like. Describe the problem that is solved, not the solution.

Sure, reworded

serhiy-storchaka · 2017-02-28T16:32:39Z

Misc/NEWS

@@ -10,6 +10,10 @@ What's New in Python 3.7.0 alpha 1?
 Core and Builtins
 -----------------

+- issue29615: xmlrpc.server.SimpleXMLRPCDispatcher.instance._dispatch function is no longer called in an except block
+  Patch by Petr MOTEJLEK


Patch by Petr Motejlek.

serhiy-storchaka · 2017-02-28T16:32:39Z

Misc/NEWS

+- issue29615: xmlrpc.server.SimpleXMLRPCDispatcher.instance._dispatch function is no longer called in an except block
+  Patch by Petr MOTEJLEK
+
+- bpo-29607: Fix stack_effect computation for CALL_FUNCTION_EX. 


Unrelated change.

Should be gone

…e and renamed tests in it to have shorter names

… uses self.assertRaises to ensure it was raised

…nd moved it higher

petr-motejlek · 2017-02-28T17:35:39Z

Hi @serhiy-storchaka , I think I addressed all your comments. Please check.

There is still the coverage issue, though. What should I do about checking the raised exception when no methods can be located, please?

In order to get 100 % coverage of the _dispatch function, though, I would also need to test the negative case when no function can be located, however the exception raised in that case is a simple Exception which is very generic. What do you suggest to do about this? Should I ignore the fact that the exception is generic and still try to catch it and make sure that what I caught was an instance of Exception and not a subclass of it? Something else?

serhiy-storchaka · 2017-02-28T18:09:27Z

I think it is enough to catch the general Exception and check that error message contains the name of the dispatched method. The type of the exception can be made more specific in future, and error message can be changed, but I hope that it will always contain the method name.

…he behavior when no function can be located by the dispatcher

…nction is None

… attribute does not exist

petr-motejlek · 2017-03-01T09:04:56Z

Ok, let's see now. I believe all comments have been addressed and there should be 100 % coverage on the updated _dispatch function

serhiy-storchaka

In general LGTM. Nice fix, nice tests, nice Misc/NEWS entry. I left few style comments, you can address them if you with, but this is not required.

serhiy-storchaka · 2017-03-01T11:03:25Z

Lib/test/test_xmlrpc.py

-            BinaryTestCase, FaultTestCase, UseBuiltinTypesTestCase,
-            SimpleServerTestCase, SimpleServerEncodingTestCase,
-            KeepaliveServerTestCase1, KeepaliveServerTestCase2,
+    support.run_unittest(XMLRPCTestCase, SimpleXMLRPCDispatcherTestCase,


I think it would be better to add new test at the end of the list of tests or at separate line. This would make the diff smaller and easier fore review.

serhiy-storchaka · 2017-03-01T11:03:25Z

Misc/NEWS

@@ -677,6 +677,10 @@ Library
 - Issue #24142: Reading a corrupt config file left configparser in an
  invalid state.  Original patch by Florian Höch.

+- bpo-29615: SimpleXMLRPCDispatcher no longer chains KeyError (or any other


Entries usually are added at the start of corresponding section.

serhiy-storchaka · 2017-03-01T11:03:25Z

Lib/test/test_xmlrpc.py

+        dispatcher.register_function(dispatched_func)
+        with self.assertRaises(self.DispatchExc) as exc_ctx:
+            dispatcher._dispatch('dispatched_func', exp_params)
+        self.assertTupleEqual(exc_ctx.exception.args, (exp_params,))


Just assertEqual() can be used. It calls assertTupleEqual() for tuples.

Darn. And here I thought you would like this :D

serhiy-storchaka · 2017-03-01T11:03:25Z

Lib/test/test_xmlrpc.py

+        chained exceptions"""
+
+    def test_call_registered_func(self):
+        """Calls explicitly registered function


Comments and docstrings that consist of multiple sentences should have periods at the end of sentences.

If you want the first line of the docstring be without a period, turn the rest of the docstring into a comment.

"""Calls explicitly registered function""" # Makes sure any exception raised inside the function has no other # exception chained to it

I will fix this, this seems reasonable

petr-motejlek · 2017-03-01T14:57:32Z

Ok, @serhiy-storchaka . I think once the sanity checks pass, we are golden.

Thank you very much for the review and patience :)

serhiy-storchaka · 2017-03-01T15:47:07Z

Lib/test/test_xmlrpc.py

-        exception chained to it"""
+        """Calls explicitly registered function"""
+        # Makes sure any exception raised inside the function has no other
+        # exception chained to it"""


""" is not needed.

Hasty typo. Removed.

serhiy-storchaka

Thank you for your contribution and patience to my endless nitpicks. ;-)

serhiy-storchaka · 2017-03-01T17:23:11Z

I think it is worth to backport this to 3.6 and 3.5. Could you please do this @petr-motejlek?

petr-motejlek · 2017-03-01T17:24:52Z

Will be my pleasure, @serhiy-storchaka. I wanted to do that in the first place, but did not know the proper procedure. What's that? Do I create separate PRs to the respective version branches?

serhiy-storchaka · 2017-03-01T17:32:53Z

Yes, just create separate PRs. Add corresponding prefix ("[3.6]") and label ("cherry-pick for 3.6").

petr-motejlek · 2017-03-05T15:27:30Z

Created 3.5 (#479) and 3.6 (#478) backport PRs.

issue29615: _dispatch function is no longer called in an except block

28b8f15

the-knights-who-say-ni added the CLA not signed label Feb 23, 2017

Mariatta removed the CLA not signed label Feb 24, 2017

the-knights-who-say-ni added the CLA signed label Feb 24, 2017

serhiy-storchaka requested changes Feb 24, 2017

View changes

issue29615 (review): use functools.wraps and remove the keyword-onl…

c8d709d

…y argument call

serhiy-storchaka requested changes Feb 27, 2017

View changes

issue29615 (review): do not chain the exceptions, refrain from attemp…

bf8c262

…ts to invoke `None`

serhiy-storchaka requested changes Feb 27, 2017

View changes

Petr MOTEJLEK added 6 commits Feb 28, 2017

issue29615 (review): remove unused functools

9124cd1

issue29615 (review): attempts to mimic the original behavior while ge…

302f4f3

…tting rid of the exception chaining

issue29615 (review): verifies KeyError`s (no exceptions actually) are…

2f7cec1

… no longer chained to exceptions raised within `_dispatch` function

issue29615 (review): entry for these changes in Misc/NEWS

3895f9b

Merge remote-tracking branch 'MASTER/master' into issue29615

d8f688c

Merge remote-tracking branch 'MASTER/master' into issue29615

8f88056

issue29615 (review): tests that ensure no exception chaining takes pl…

240bf23

…ace when calling dispatched funcs

serhiy-storchaka requested changes Feb 28, 2017

View changes

Petr MOTEJLEK added 10 commits Feb 28, 2017

issue29615 (review): created a separate SimpleXMLRPCDispatcherTestCas…

aee470e

…e and renamed tests in it to have shorter names

issue29615 (review): defines test case level DispatchExc and uses it,…

ffb8317

… uses self.assertRaises to ensure it was raised

issue29615 (review): proper assert test

8d9396e

issue29615 (review): removed the assert delegation from DispatchExc a…

17bbcea

…nd moved it higher

issue29615 (review): removed keyword arguments when calling _dispatch

d6d8bba

issue29615 (review): no longer staticmethod

3efea6c

issue29615 (review): removed unnecessary else block

5fc1a7d

issue29615 (review): fewer raises

4f54e7d

issue29615 (review): reformulated and moved to the appropriate place

73b9fe9

issue29615 (review): removed

58c32d2

Petr MOTEJLEK added 5 commits Mar 1, 2017

issue29615 (review): uses Exception.args; shorter lines

19af5ec

issue29615 (review): consisten use if if condition

97d5e17

issue29615 (review): uses assertTupleEqual to compare tuples; tests t…

5d30bc8

…he behavior when no function can be located by the dispatcher

issue29615 (review): tests what happens when explicitly registered fu…

8b63473

…nction is None

issue29615 (review): checks what happens when a registered instance's…

95af409

… attribute does not exist

serhiy-storchaka self-assigned this Mar 1, 2017

serhiy-storchaka approved these changes Mar 1, 2017

View changes

Petr MOTEJLEK added 5 commits Mar 1, 2017

issue29615 (review): typography

6f487e5

issue29615 (review): uses assertEqual instead of assertTupleEqual

0869e40

issue29615 (review): shorter diff

71e81d2

Merge remote-tracking branch 'MASTER/master' into issue29615

bbee467

issue29615 (review): moving the NEWS item to the proper place

1823a4a

serhiy-storchaka reviewed Mar 1, 2017

View changes

issue29615 (review): typo

e3b7642

serhiy-storchaka approved these changes Mar 1, 2017

View changes

serhiy-storchaka added needs backport to 3.5 labels Mar 1, 2017

serhiy-storchaka merged commit 3c6314c into python:master Mar 1, 2017

serhiy-storchaka changed the title ~~issue29615: _dispatch function is no longer called in an except block~~ bpo-29615: SimpleXMLRPCDispatcher no longer chains KeyError (or any other exception) to exception(s) raised in the dispatched methods. Mar 1, 2017

Mariatta removed needs backport to 3.5 labels Mar 5, 2017

serhiy-storchaka removed their assignment Dec 6, 2018

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Bump redis from 3.3.4 to 3.3.6 (#260)

159aa81

bpo-29615: SimpleXMLRPCDispatcher no longer chains KeyError (or any other exception) to exception(s) raised in the dispatched methods. #260

bpo-29615: SimpleXMLRPCDispatcher no longer chains KeyError (or any other exception) to exception(s) raised in the dispatched methods. #260

Conversation

petr-motejlek commented Feb 23, 2017

the-knights-who-say-ni commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petr-motejlek Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petr-motejlek commented Feb 28, 2017 • edited

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petr-motejlek commented Feb 28, 2017

serhiy-storchaka commented Feb 28, 2017

petr-motejlek commented Mar 1, 2017

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petr-motejlek commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

serhiy-storchaka commented Mar 1, 2017

petr-motejlek commented Mar 1, 2017

serhiy-storchaka commented Mar 1, 2017

petr-motejlek commented Mar 5, 2017

petr-motejlek Feb 27, 2017 •

edited

petr-motejlek commented Feb 28, 2017 •

edited