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 <robbie.gemm...@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-unsubscr...@qpid.apache.org >> For additional commands, e-mail: commits-h...@qpid.apache.org >>
