> On Jul 14, 2014, at 16:21, Lee Skillen <lskil...@vulcanft.com> wrote: > >> On 10 July 2014 14:25, Andi Vajda <va...@apache.org> wrote: >> >>> 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.. > > Great - I created PYLUCENE-30 to help track it further (hope that's > acceptable).
Excellent, thanks ! > Let me know if you need any other help or additional input! I'm on vacation at the moment so things are moving slowly... 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> > > > > -- > Lee Skillen > > Vulcan Financial Technologies > 1st Floor, 47 Malone Road, Belfast, BT9 6RY > > Office: +44 (0)28 95 817888 > Web: www.vulcanft.com
