Hi Bozzo, some comments and questions.

I am seeing test failures due to NPE's from the new code:
https://builds.apache.org/view/M-R/view/Qpid/job/Qpid-proton-j/1043/

There should be null checks on the remote Source and Target before
trying to copy them, since there are cases where they are allowed and
expected to be null (though in the failing tests its not clear the
they fall in to those, so much as perhaps not bothering to set them as
they werent of interest for the test).

I see you listed the change as a bug, but I can't say I was ever under
the impression it should do this (though I can certainly see it makes
life easier in some cases). Does proton-c do this and proton-j did
not?

If we are doing this for Source and Target objects, should it not also
work for the Coordinator? Seems a bit unfair to leave it out given the
method was added.

Please don't use if statements without braces, someone will inevitably
screw it up when updating things later :)

Robbie

On 7 July 2015 at 20:50,  <bo...@apache.org> wrote:
> Repository: qpid-proton
> Updated Branches:
>   refs/heads/master 09af37524 -> f6d74a47d
>
>
> PROTON-937: LinkImpl.localOpen() does not initialize source and target
>
>
> Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
> Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/f6d74a47
> Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/f6d74a47
> Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/f6d74a47
>
> Branch: refs/heads/master
> Commit: f6d74a47d1f3f4ee3f4f3a444e239a209276f928
> Parents: d4d22ee
> Author: Bozo Dragojevic <bo...@digiverse.si>
> Authored: Tue Jul 7 21:32:58 2015 +0200
> Committer: Bozo Dragojevic <bo...@digiverse.si>
> Committed: Tue Jul 7 21:49:44 2015 +0200
>
> ----------------------------------------------------------------------
>  .../qpid/proton/amqp/messaging/Source.java       | 19 +++++++++++++++++++
>  .../qpid/proton/amqp/messaging/Target.java       | 12 ++++++++++++
>  .../qpid/proton/amqp/messaging/Terminus.java     | 17 +++++++++++++++++
>  .../proton/amqp/transaction/Coordinator.java     |  5 +++++
>  .../qpid/proton/amqp/transport/Source.java       |  2 ++
>  .../qpid/proton/amqp/transport/Target.java       |  2 ++
>  .../apache/qpid/proton/engine/impl/LinkImpl.java |  4 ++++
>  7 files changed, 61 insertions(+)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Source.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Source.java 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Source.java
> index 5efc15a..e6fffef 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Source.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Source.java
> @@ -21,7 +21,9 @@
>  package org.apache.qpid.proton.amqp.messaging;
>
>  import java.util.Arrays;
> +import java.util.HashMap;
>  import java.util.Map;
> +
>  import org.apache.qpid.proton.amqp.Symbol;
>
>  public final class Source extends Terminus
> @@ -32,6 +34,18 @@ public final class Source extends Terminus
>      private Outcome _defaultOutcome;
>      private Symbol[] _outcomes;
>
> +    private Source(Source other) {
> +        super(other);
> +        _distributionMode = other._distributionMode;
> +        if (other._filter != null)
> +            _filter = new HashMap(other._filter);
> +        _defaultOutcome = other._defaultOutcome;
> +        if (other._outcomes != null)
> +            _outcomes = other._outcomes.clone();
> +    }
> +
> +    public Source() {}
> +
>      public Symbol getDistributionMode()
>      {
>          return _distributionMode;
> @@ -90,5 +104,10 @@ public final class Source extends Terminus
>                 ", capabilities=" + (getCapabilities() == null ? null : 
> Arrays.asList(getCapabilities())) +
>                 '}';
>      }
> +
> +    @Override
> +    public org.apache.qpid.proton.amqp.transport.Source copy() {
> +        return new Source(this);
> +    }
>  }
>
> \ No newline at end of file
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Target.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Target.java 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Target.java
> index 1749d40..38678d1 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Target.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Target.java
> @@ -28,6 +28,13 @@ import java.util.Arrays;
>  public final class Target extends Terminus
>        implements org.apache.qpid.proton.amqp.transport.Target
>  {
> +    private Target(Target other) {
> +        super(other);
> +    }
> +
> +    public Target() {
> +    }
> +
>      @Override
>      public String toString()
>      {
> @@ -41,5 +48,10 @@ public final class Target extends Terminus
>                 ", capabilities=" + (getCapabilities() == null ? null : 
> Arrays.asList(getCapabilities())) +
>                 '}';
>      }
> +
> +    @Override
> +    public org.apache.qpid.proton.amqp.transport.Target copy() {
> +        return new Target(this);
> +    }
>  }
>
> \ No newline at end of file
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Terminus.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Terminus.java 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Terminus.java
> index be57957..ac28b32 100644
> --- 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Terminus.java
> +++ 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/messaging/Terminus.java
> @@ -20,7 +20,9 @@
>   */
>  package org.apache.qpid.proton.amqp.messaging;
>
> +import java.util.HashMap;
>  import java.util.Map;
> +
>  import org.apache.qpid.proton.amqp.Symbol;
>  import org.apache.qpid.proton.amqp.UnsignedInteger;
>
> @@ -37,6 +39,21 @@ public abstract class Terminus
>      Terminus()
>      {
>      }
> +
> +    protected Terminus(Terminus other) {
> +        _address = other._address;
> +        _durable = other._durable;
> +        _expiryPolicy = other._expiryPolicy;
> +        _timeout = other._timeout;
> +        _dynamic = other._dynamic;
> +        if (other._dynamicNodeProperties != null) {
> +            // TODO: Do we need to copy or can we make a simple reference?
> +            _dynamicNodeProperties = new 
> HashMap(other._dynamicNodeProperties); // FIXME
> +        }
> +        if (other._capabilities != null) {
> +            _capabilities = other._capabilities.clone(); // FIXME?
> +        }
> +    }
>
>      public final String getAddress()
>      {
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/amqp/transaction/Coordinator.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/transaction/Coordinator.java
>  
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/transaction/Coordinator.java
> index 2af968d..7ff000a 100644
> --- 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/transaction/Coordinator.java
> +++ 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/transaction/Coordinator.java
> @@ -55,5 +55,10 @@ public final class Coordinator
>      {
>          return null;
>      }
> +
> +    @Override
> +    public Target copy() {
> +        return null;
> +    }
>  }
>
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Source.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Source.java 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Source.java
> index 93d71f7..2d6f3b2 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Source.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Source.java
> @@ -23,4 +23,6 @@ package org.apache.qpid.proton.amqp.transport;
>  public interface Source
>  {
>      public String getAddress();
> +
> +    public Source copy();
>  }
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Target.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Target.java 
> b/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Target.java
> index 8b81f37..c972c02 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Target.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/amqp/transport/Target.java
> @@ -24,4 +24,6 @@ package org.apache.qpid.proton.amqp.transport;
>  public interface Target
>  {
>      public String getAddress();
> +
> +    public Target copy();
>  }
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/f6d74a47/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/LinkImpl.java
> ----------------------------------------------------------------------
> diff --git 
> a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/LinkImpl.java 
> b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/LinkImpl.java
> index 6b63b9a..ca98096 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/LinkImpl.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/LinkImpl.java
> @@ -418,6 +418,10 @@ public abstract class LinkImpl extends EndpointImpl 
> implements Link
>      @Override
>      void localOpen()
>      {
> +        if (_source == null)
> +            _source = _remoteSource.copy();
> +        if (_target == null)
> +            _target = _remoteTarget.copy();
>          getConnectionImpl().put(Event.Type.LINK_LOCAL_OPEN, this);
>      }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org
> For additional commands, e-mail: commits-h...@qpid.apache.org
>

Reply via email to