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
