search

Exa-Networks / exaproxy

Performant Content Modifying Non-Caching Proxy [stable - accepting patches for bug fixes only]
Other
145 stars 36 forks source link

No longer capture decideHTTP errors #23

Closed readevalprint closed 10 years ago

readevalprint commented 10 years ago

One more.... :D Now this one changes the behavior a bit (for the better I hope).

For some context on what is happening, I'm getting intermittent exceptions of UnboundLocalError: local variable 'decision' referenced before assignment and looking over it, the only way this can happen is if decideICAP raises an exception that was then caught and printed then tries to return decision. And so I wondered what the original exception was. A bit of judicial debugging shows e is AttributeError("'HTTP' object has no attribute 'upgrade'",).

So seeing how an exception in decideHTTP will still raise an exception, it makes sense to let the original one bubble up.

david-farrar commented 10 years ago

Hello,

Thank you for the patch - and for making me realise that I'd left some debugging code in there to print any errors I was testing for :D

The exception you're seeing suggests that the proxy was unable to parse your HTTP request since successful validation would always result in the 'upgrade' attribute being set. I'd neglected to actually return the response created after a parse error is detected so that would explain how this was happening: our latest patch should fix the behaviour.

Could you please let us know if this not the case?

readevalprint commented 10 years ago

@david-farrar I see the changes in #24 and will give them a try now. Also I don't think this PR merged correctly. Did you still want to merge it or was that a mistake?

https://github.com/david-farrar/exaproxy/blob/02c273ab00aee71908f2fa8e2698960c70751d7e/lib/exaproxy/reactor/redirector/icap.py#L191

thomas-mangin commented 10 years ago

'' and None are semantically different so your patch needed a little tweaking. I believe it is normal and was due to the way the patches were merged.

thomas-mangin commented 10 years ago

And thank you for your help

readevalprint commented 10 years ago

@thomas-mangin sure I understand the need for tweaks but what I mean is this PR #23 was just merged when it doesn't make sense to anymore. You can see in the current master branch (02c273ab00aee71908f2fa8e2698960c70751d7e) that response_string is assigned redundantly, decision is never returned, and source == 'proxy' is compared twice. And glad to help! Thanks for writing exaproxy.

david-farrar commented 10 years ago

Whoops.

I'd accidentally left my own change to that bit of code in the previous patch and had to manually fix the conflicts caused in your pull request. Apparently I should just have got some sleep instead :)

readevalprint commented 10 years ago

Haha no worries, always opt for sleep. Thanks for fixing this.