> On Jul 10, 2014, at 12:23, Lee Skillen <lskil...@vulcanft.com> wrote: > > Hey, > >> On 9 July 2014 18:38, Andi Vajda <va...@apache.org> wrote: >> >>> On Wed, 9 Jul 2014, Andi Vajda wrote: >>> >>> >>> >>>> On Wed, 9 Jul 2014, Lee Skillen wrote: >>>> >>>> Hey Andi, >>>> >>>>> On 9 July 2014 13:50, Andi Vajda <va...@apache.org> wrote: >>>>> >>>>> >>>>> >>>>> Hi Lee, >>>>> >>>>> >>>>>> On Tue, 8 Jul 2014, Lee Skillen wrote: >>>>>> >>>>>> Andi, thanks for the reply - I've created a github mirror of the >>>>>> pylucene >>>>>> project for our own use (which I intend to keep synced with your SVN >>>>>> repository as its official upstream), located at: >>>>>> >>>>>> https://github.com/lskillen/pylucene >>>>>> >>>>>> As suggested I have formatted (and attached) a patch of the unfinished >>>>>> code >>>>>> that we're using for the through-layer exceptions. Alternatively the >>>>>> diff >>>>>> can be inspected via github by diff'ing between the new >>>>>> feature-thru-exception branch that I have created and the master >>>>>> branch, as >>>>>> such: >>>>>> >>>>>> >>>>>> https://github.com/lskillen/pylucene/compare/feature-thru-exception?expand=1 >>>>>> >>>>>> Although we've run the test suite without issues I realise there may >>>>>> still >>>>>> be functionality/style/logical issues with the code. I also suspect >>>>>> that >>>>>> there may not be specific test cases that target regression failures >>>>>> for >>>>>> exceptions (yet), so confidence isn't high! All comments are welcome >>>>>> and I >>>>>> realise this will likely require further changes and a repeatable test >>>>>> case >>>>>> before it is acceptable, but that's fine. >>>>> >>>>> >>>>> >>>>> I took a look at your patch and used the main idea to rewrite it so >>>>> that: >>>>> - ref handling is correct (hopefully): you had it backwards when >>>>> calling >>>>> Py_Restore(), it steals what you currently must own. >>>>> - the Python error state is now saved as one tuple of (type, value, >>>>> tb) on >>>>> PythonException, not as three strings and three fake Objects >>>>> (longs). >>>>> - ref handling is correct via a finalize() method DECREF'ing the saved >>>>> error state tuple on PythonException when that exception is >>>>> collected. >>>>> - getMessage() still works as before but the traceback is extracted on >>>>> demand, not at throw time as was done before. >>>>> >>>>> The previous implementation was done so that no Python cross-VM >>>>> reference had to be tracked, all the error data was string'ified at error >>>>> time. >>>>> Your change now requires that the error be kept 'alive' accross layers >>>>> and refs must be tracked to avoid leaks. >>>> >>>> Great feedback, thank you - I suspected that the reference handling >>>> wasn't quite there (first foray into CPython internals), and I also >>>> really wanted to utilise a tuple and get rid of the strings but wasn't >>>> sure what the standard was for extending a class such as >>>> PythonException. I actually did a quick attempt at running everything >>>> under debug mode to inspect for leaks, but suffice to say that my >>>> usual toolbox for debugging and tracking memory leaks >>>> (gdb/clang/valgrind) didn't work too well - Had some minor success >>>> with the excellent objgraph Python package, but would need to spend >>>> more time on it. >>>> >>>>> >>>>> I did _not_ test this as I don't have a system currently running that >>>>> uses JCC in reverse like this. Since you're actively using the feature, >>>>> please test the attached patch (against trunk) and report back with >>>>> comments, bugs, fixes, etc... >>>> >>>> Not a problem! I applied your modified patch against trunk, rebuilt >>>> and re-ran our application. The good news is that everything is >>>> working really well, and the only bug that I could see was within the >>>> RegisterNatives() call within registerNatives() for PythonException >>>> (size was still 2, changed it to calculate the size of the struct). >>>> I'll re-attach the patch with the changes for you to review. >>> >>> >>> Woops, I missed that - even though I looked for it. Oh well. Thanks. >> >> >> I attached a new patch with your fix. I also removed the 'clear()' method >> since it's no longer necessary: once the PythonException is constructed, the >> error state in the Python VM is cleared because of PyErr_Fetch() being >> called during saveErrorState(). > > Changes are looking great - Applied against trunk again here and all > working well. I've also added a new (simple) test case within the > test directory under the pylucene root (test_PythonException.py) that > checks to see if the through-layer exceptions are working, > realistically it should probably be checking the traceback as well but > I think this would suffice to show that the exceptions are propagating > properly. On a side note, I did have an issue building pylucene > because java/org/apache/pylucene/store/PythonIndexOutput.java refused > to build, as it was referencing a BufferedIndexOutput that seems to > have been removed by LUCENE-5678 (I just removed it in my local trunk > but didn't want to add that to the patch).
Yes, that's fixed in the upcoming release 4.9.0 available to be voted on. See thread on this list started a few days ago for more details. I should try and integrate your testcase next... Thanks ! Andi.. > > Cheers, > Lee > >> >> Andi.. >> >> >>> >>>> The only other comments I have are that: >>>> >>>> (1) This was still an issue with my patch, but I don't know if there >>>> is anyone out there relying upon the exact format of the string that >>>> gets returned by getMessage(), as it is probably going to be different >>>> now that it is calculated at a different point - In saying that I >>>> imagine you would only be doing that if you were specifically trying >>>> to handle an exception from the Python layer and were trying to >>>> re-create it using the string, which wouldn't be an issue now. >>> >>> >>> Yeah, I'm just concatenating the strings. I'm already using PyErr_Print(). >>> >>>> (2) I also noticed that your patch doesn't have the #if >>>> defined(PYTHON) part to protect certain parts that rely on things like >>>> getPythonExceptionClass() - Not sure if that was intentional or not >>>> (guessing it might be one of those things that are always defined >>>> anyway), but just wanted to highlight it. >>> >>> >>> Not needed, all of functions.cpp depends on python amyway. >>> >>>> Apart from that, it looks great - In order to assist with replication >>>> of the through-exception issue on your side I also whipped up a (very) >>>> quick example and am also attaching this to the email. Utilises a >>>> noddy Makefile to get the job done, but running "make" or "make test" >>>> will generate the java/jcc packages and then run the test. Running >>>> "make uninstall" will remove the jcc package, and "make clean" will >>>> tidy up the build artifacts. Standard stuff! >>>> >>>> Hope that helps! >>> >>> >>> Excellent, thanks ! >>> >>> Andi.. >>> >>>> >>>> Thanks, >>>> Lee >>>> >>>>> Thanks ! >>>>> >>>>> Andi.. >>>>> >>>>> >>>>>> >>>>>> Thanks, >>>>>> Lee >>>>>> >>>>>> >>>>>> >>>>>>> On 4 July 2014 18:17, Andi Vajda <va...@apache.org> wrote: >>>>>>> >>>>>>> >>>>>>> On Jul 4, 2014, at 18:33, Lee Skillen <lskil...@vulcanft.com> wrote: >>>>>>> >>>>>>> Hi Andi, >>>>>>> >>>>>>> First of all, absolutely fantastic work on the JCC project, we love it >>>>>>> - >>>>>>> Second of all, apologies for contacting you out of the blue via email, >>>>>>> but >>>>>>> I wasn't sure of the best place to contact you to ask a few questions >>>>>>> (technical-level, not support-level). If it isn't acceptable to email >>>>>>> please let me know and I can move the discussion somewhere else. >>>>>>> >>>>>>> >>>>>>> Yes, please include pylucene-dev@lucene.apache.org (subscription >>>>>>> required. send mail pylucene-dev-subscribe@ and follow the >>>>>>> instructions >>>>>>> in the reply). >>>>>>> >>>>>>> A bit of background first of all :- >>>>>>> >>>>>>> We've been integrating JCC for the past two weeks on a sizeable >>>>>>> Python/Java/C++ project (where Python is a controller/glue layer) >>>>>>> having >>>>>>> migrated from a previous solution to integrate across the languages, >>>>>>> and >>>>>>> for the most part JCC has been a lifesaver. >>>>>>> >>>>>>> Technically we've only hit one major issue so far and that has been >>>>>>> the >>>>>>> translation of exceptions through the layers. Our Java application >>>>>>> does a >>>>>>> lot of double-dispatch (visitation) type of execution and we're >>>>>>> calling >>>>>>> outwards to Python to implement the concrete visitation logic. >>>>>>> >>>>>>> In the case of an exception being thrown it results in JCC collating a >>>>>>> traceback and captured exception at every level of scope, resulting in >>>>>>> a >>>>>>> giant JavaError exception which has lost it's original PythonException >>>>>>> context. >>>>>>> >>>>>>> I was actually able to fix this by attempting to capture the full >>>>>>> context >>>>>>> via PyErr_Fetch() when the first python object is thrown and storing >>>>>>> it >>>>>>> within PythonException, and then ensuring that this is carried through >>>>>>> each >>>>>>> layer appropriately - This seems to work very well, although >>>>>>> admittedly I >>>>>>> haven't considered regressive errors or compatibility yet, but just >>>>>>> wanted >>>>>>> to proof-of-concept it first of all then speak with a JCC maintainer. >>>>>>> >>>>>>> The net result is being able to catch errors in a pipeline similar to >>>>>>> the >>>>>>> following: >>>>>>> >>>>>>> Python >>>>>>> -> Java >>>>>>> -> Python >>>>>>> -> Raise MyException >>>>>>> <- Throw PythonException (containing MyException) >>>>>>> <- Inject MyException >>>>>>> Catch MyException >>>>>>> >>>>>>> My main questions are to ask your thoughts about :- >>>>>>> >>>>>>> - What are your thoughts about through-layer exceptions and a feature >>>>>>> improvement like this? >>>>>>> >>>>>>> >>>>>>> I agree that losing information during exception throwing is not good. >>>>>>> If >>>>>>> you've got an improvement in the form of a patch, do not hesitate in >>>>>>> sending it in. >>>>>>> >>>>>>> - JCC is (I think) currently at home with pylucene, are there are >>>>>>> plans to >>>>>>> making it separate? >>>>>>> >>>>>>> >>>>>>> No such plans at the moment. >>>>>>> >>>>>>> - Are you happy if I create a public github project and add the >>>>>>> patched >>>>>>> code in there for review? >>>>>>> >>>>>>> >>>>>>> You're welcome to fork the code if you'd like but you don't have to if >>>>>>> all >>>>>>> you want is sending a patch. Your call. >>>>>>> >>>>>>> Thank you for the kind words ! >>>>>>> >>>>>>> Andi.. >>>>>>> >>>>>>> >>>>>>> Hope to hear from you! >>>>>>> >>>>>>> Cheers, >>>>>>> Lee >>>>>>> >>>>>>> >>>>>>> >>>>>>> TL;DR; >>>>>>> >>>>>>> We would like to :- >>>>>>> >>>>>>> - Improve JCC's through-layer exception support. >>>>>>> - Establish a github project for JCC alone. >>>>>>> >>>>>>> -- >>>>>>> Lee Skillen >>>>>>> >>>>>>> Vulcan Financial Technologies >>>>>>> 1st Floor, 47 Malone Road, Belfast, BT9 6RY >>>>>>> >>>>>>> Office: +44 (0)28 95 817888 >>>>>>> Mobile: +44 (0)78 41 425152 >>>>>>> Web: www.vulcanft.com >>>>>> >>>>>> >>>>>> -- >>>>>> Lee Skillen >>>>>> >>>>>> Vulcan Financial Technologies >>>>>> 1st Floor, 47 Malone Road, Belfast, BT9 6RY >>>>>> >>>>>> Office: +44 (0)28 95 817888 >>>>>> Mobile: +44 (0)78 41 425152 >>>>>> Web: www.vulcanft.com >>>> >>>> >>>> >>>> >>>> >>>> -- >>>> Lee Skillen >>>> >>>> Vulcan Financial Technologies >>>> 1st Floor, 47 Malone Road, Belfast, BT9 6RY >>>> >>>> Office: +44 (0)28 95 817888 >>>> Web: www.vulcanft.com > > > > -- > Lee Skillen > > Vulcan Financial Technologies > 1st Floor, 47 Malone Road, Belfast, BT9 6RY > > Office: +44 (0)28 95 817888 > Web: www.vulcanft.com > <feature-thru-exception-3.patch>
