You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by Robbie Gemmell <ro...@gmail.com> on 2015/07/08 18:16:50 UTC
Re: [1/2] qpid-proton git commit: PROTON-937: LinkImpl.localOpen()
does not initialize source and target
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-unsubscribe@qpid.apache.org
> For additional commands, e-mail: commits-help@qpid.apache.org
>
Re: [1/2] qpid-proton git commit: PROTON-937: LinkImpl.localOpen()
does not initialize source and target
Posted by Robbie Gemmell <ro...@gmail.com>.
Thinking this through further, there are also cases where you actually
want to set the source/target to be null when the remote versions were
originally not, which this change would somewhat break.
I think we need to revert this for now and look to do something after
the release, once the approach is clearer and it is actually tested
etc.
The NPEs from the change are also causing the CI jobs to hang, I just
killed one running since yesterday (and updated the job to have a
timeout to ensure it doesnt happen again),
https://builds.apache.org/view/M-R/view/Qpid/job/Qpid-proton-c/716.
It got this far and then nothing else was logged. Obviously this might
also point to an issue with the new reactor bits and its tests.
proton_tests.reactor_interop.ReactorInteropTest. \
test_protonj_to_protonc_1
...........................................org.apache.qpid.proton.engine.HandlerException:
java.lang.NullPointerException
at org.apache.qpid.proton.engine.impl.EventImpl.dispatch(EventImpl.java:212)
at org.apache.qpid.proton.reactor.impl.ReactorImpl.process(ReactorImpl.java:269)
at org.apache.qpid.proton.reactor.impl.ReactorImpl.run(ReactorImpl.java:332)
at org.apache.qpid.proton.ProtonJInterop.main(ProtonJInterop.java:190)
Caused by: java.lang.NullPointerException
at org.apache.qpid.proton.engine.impl.LinkImpl.localOpen(LinkImpl.java:422)
at org.apache.qpid.proton.engine.impl.EndpointImpl.open(EndpointImpl.java:74)
at org.apache.qpid.proton.ProtonJInterop$SendHandler.onConnectionInit(ProtonJInterop.java:66)
at org.apache.qpid.proton.engine.impl.EventImpl.dispatch(EventImpl.java:88)
... 3 more
Tweaking things to remove the NPE, I still see another test failure in
the maven build:
proton_tests.reactor.ExceptionTest.test_schedule_cancel ................. fail
Error during test: Traceback (most recent call last):
File "/home/gemmellr/workspace/proton/tests/python/proton-test",
line 360, in run
phase()
File "/home/gemmellr/workspace/proton/tests/python/proton_tests/reactor.py",
line 181, in test_schedule_cancel
now = self.reactor.mark()
File "/home/gemmellr/workspace/proton/tests/../proton-c/bindings/python/proton/reactor.py",
line 118, in mark
return pn_reactor_mark(self._impl)
NameError: global name 'pn_reactor_mark' is not defined
Robbie
On 8 July 2015 at 17:16, Robbie Gemmell <ro...@gmail.com> wrote:
> 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-unsubscribe@qpid.apache.org
>> For additional commands, e-mail: commits-help@qpid.apache.org
>>
Fwd: [1/2] qpid-proton git commit: PROTON-937: LinkImpl.localOpen()
does not initialize source and target
Posted by Robbie Gemmell <ro...@gmail.com>.
Sending to the list...
---------- Forwarded message ----------
From: Robbie Gemmell <ro...@gmail.com>
Date: 9 July 2015 at 12:35
Subject: Re: [1/2] qpid-proton git commit: PROTON-937:
LinkImpl.localOpen() does not initialize source and target
To: Bozo Dragojevic <bo...@digiverse.si>
On 9 July 2015 at 12:23, Bozo Dragojevic <bo...@digiverse.si> wrote:
> On 8. 07. 15 18.16, Robbie Gemmell wrote:
>> 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?
>>
>
> I reverted this, and will do it properly.
>
Great, thanks.
> Proton-c does do it in the handshaker and in the messenger.
> Just do a git grep pn_terminus_copy. And yes, it has a check so it
> doesn't try to copy non-existing stuff. In which case the copy returns
> an error, which handshaker ignores :)
>
> For some unexplicable reason I thought the LinkImpl.localOpen was a
> better place to do it while it's perfectly easy to do in in the
> Handshaker.onLinkRemoteOpen() and the messenger already does it in a way.
>
> //TODO: the following is not correct; should only copy those
> properties that we understand
> link.setSource(link.getRemoteSource());
> link.setTarget(link.getRemoteTarget());
>
Yes, doing it in Handshaker / Messenger is quite different than doing
so within the link object itself, and more in line with what I would
expect. The comment in the above snippet is another reason doing it in
the link would cause issues.
>> 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.
>
> I've yet to come up-to-speed with the Coordinator, so cannot comment here :)
>
>> Please don't use if statements without braces, someone will inevitably
>> screw it up when updating things later :)
>>
> Agree, too much switching between python and Java and C in one day :)
>
>
> Bozzo
Re: [1/2] qpid-proton git commit: PROTON-937: LinkImpl.localOpen()
does not initialize source and target
Posted by Bozo Dragojevic <bo...@digiverse.si>.
On 8. 07. 15 18.16, Robbie Gemmell wrote:
> 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?
>
I reverted this, and will do it properly.
Proton-c does do it in the handshaker and in the messenger.
Just do a git grep pn_terminus_copy. And yes, it has a check so it
doesn't try to copy non-existing stuff. In which case the copy returns
an error, which handshaker ignores :)
For some unexplicable reason I thought the LinkImpl.localOpen was a
better place to do it while it's perfectly easy to do in in the
Handshaker.onLinkRemoteOpen() and the messenger already does it in a way.
//TODO: the following is not correct; should only copy those
properties that we understand
link.setSource(link.getRemoteSource());
link.setTarget(link.getRemoteTarget());
> 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.
I've yet to come up-to-speed with the Coordinator, so cannot comment here :)
> Please don't use if statements without braces, someone will inevitably
> screw it up when updating things later :)
>
Agree, too much switching between python and Java and C in one day :)
Bozzo