Re: Review Request 45389: WIP PROTON-1133 - Avoid using hostname as a transport address

[Robbie Gemmell <ro...@gmail.com>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45389/#review126652
-----------------------------------------------------------



Actually, I had some more thoughts...

By the connection creation method now taking a 'url' argument it somewhat implies that the scheme will have an effect, which it doesn't appear to other than influencing the default port if none was specified. We probably want to avoid repeating a certain recent situation like that. Would using separate host and port args be better there?

I realised that the 'attachments' are being used to store the URL in the C connection. It might be worth doing the same on the Java side, that would presumably help achieve what my earlier comments were aimed at, avoiding reactor-specific stuff leaking out into the interfaces, while also avoiding the hacky cast-to-impl hack in the background.

Finally, I now realise that whilst this change would let people do things in a new way that will send the correct hostname value in the open etc frames, anyone continuing to do it the old way (perhaps due to not realising the new way even exists, since we are maintaining compatibility) will keep sending the wrong value as that will remain broken. I was just testing something out with the reactor againt the Java broker, and came to realise this 'open hostname includes port' issue breaks things against it fairly comprehensively because the broker cant determine the virtualhost. I worked around it by having the reactor bits reset the connection hostname field to exclude the port after parsing its value out while doing the connect call. Obviously changing the value under the covers isn't at all nice, but I do wonder if it is perhaps still better than the alternative of continuing to send the incorrect values.

- Robbie Gemmell


On March 29, 2016, 8:27 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45389/
> -----------------------------------------------------------
> 
> (Updated March 29, 2016, 8:27 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Justin Ross, and Robbie Gemmell.
> 
> 
> Bugs: PROTON-1133
>     https://issues.apache.org/jira/browse/PROTON-1133
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> This involves a change to the Reactor API.
> 
> This patch implements a new API for creating outgoing connections via the Reactor class: pn_reactor_connection_to_url().  It is meant to replace the existing practice of creating a connection via pn_reactor_connection() then setting the transport address in the connection's hostname field.
> 
> Not 100% convinced this is appropriate.  It introduces a URL parameter to the Proton Connection object, where it may make better sense to associate the URL with the Transport instead (pn_reactor_transport_to_url()???).
> 
> The URL parameter is used by the Proton iohandler to create the socket connection.  If an application does not use the Proton iohandler (by overriding the reactor's global handler), then it is the responsiblity of whatever handler is being provided to use the URL to set up the socket connection.  This was also the case for the old method that used the connection's hostname setting, so this is not a behavioral change.
> 
> 
> Diffs
> -----
> 
>   examples/java/reactor/src/main/java/org/apache/qpid/proton/example/reactor/Send.java 22da720 
>   examples/python/reactor/send.py c718780 
>   examples/python/reactor/tornado-send.py 54b8618 
>   proton-c/bindings/python/proton/reactor.py cda6248 
>   proton-c/include/proton/reactor.h e91b169 
>   proton-c/src/reactor/connection.c 4a57bfd 
>   proton-c/src/tests/reactor.c 1e706e2 
>   proton-j/src/main/java/org/apache/qpid/proton/engine/Connection.java feff80b 
>   proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ConnectionImpl.java b708d83 
>   proton-j/src/main/java/org/apache/qpid/proton/reactor/Reactor.java 9d67d49 
>   proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/IOHandler.java 40eddac 
>   proton-j/src/main/java/org/apache/qpid/proton/reactor/impl/ReactorImpl.java 0eb126a 
>   proton-j/src/test/java/org/apache/qpid/proton/reactor/ReactorTest.java 10c591a 
>   tests/java/org/apache/qpid/proton/ProtonJInterop.java 31306ef 
> 
> Diff: https://reviews.apache.org/r/45389/diff/
> 
> 
> Testing
> -------
> 
> Updated unit tests and re-checked modified examples.
> 
> 
> Thanks,
> 
> Kenneth Giusti
> 
>