qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robbie Gemmell <robbie.gemm...@gmail.com>
Subject Re: Review Request 45389: WIP PROTON-1133 - Avoid using hostname as a transport address
Date Fri, 01 Apr 2016 21:52:28 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message