Sending to the list... ---------- Forwarded message ---------- From: Robbie Gemmell <robbie.gemm...@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
