On Tue, 11 Jan 2022 at 09:35, Pete Fawcett <p...@fawcett.co.uk> wrote:
> Hi Gordon > > Thank you for the helpful response > > On Fri, 7 Jan 2022 at 11:18, Gordon Sim <g...@redhat.com> wrote: > >> On Thu, Jan 6, 2022 at 8:56 PM Pete Fawcett <p...@fawcett.co.uk> wrote: >> > *Questions:* >> > >> > Firstly, a simple work around is to check the link pointer when moving >> > delivery pointers from 'pending' to completed and discarding them if the >> > 'link' is NULL . Does this suffice? >> >> I think that would probably work. However it would be nicer to >> actually remove them from pending before they were invalidated. Does >> calling abort_pending() for the link before closing it in >> >> https://github.com/apache/qpid-cpp/blob/main/src/qpid/broker/amqp/Session.cpp#L764 >> prevent the segfault? >> > > I will try this. Thank you for pointing out where to remove the pending > items, I hadn't managed to figure that out. > I tried this but it didn't, initially, fix the problem. It turns out that the current exception handling is causing the link to be closed from within Connection object doDeliveryUpdated <https://github.com/apache/qpid-cpp/blob/main/src/qpid/broker/amqp/Connection.cpp#L675> Calling Session::abort_pending() from there meant changing abort_pending() to be a public function, which makes me wonder if it is the right thing to be doing. Making the change, and calling abort_pending() before closing the link in doDeliveryUpdated, did remove the pending items and so prevent the Segfault. As well as being concerned about changing functions to be part of the public interface, I would still have concerns that other cases may arise. My instinct would be to keep my initial suggestion of checking for invalid pending deliveries before they are actioned. This would be in addition to these attempts to clear the deliveries when the link closes. I haven't yet looked at the prospect of not closing the link for the particular case of a full queue, but I think the handling of the link error needs to be "safe", regardless of other possible changes. > > > >> > Secondly, does the link need to be cleared just because a message is too >> > big for the queue? Can it not be signalled back to the sender in a less >> > drastic manner? (Dropping the link means it is likely for numerous >> messages >> > to be repeatedly delivered when using "at least once" delivery) >> >> The message could be rejected. (I think that would involve changing >> the exception handling at >> >> https://github.com/apache/qpid-cpp/blob/main/src/qpid/broker/amqp/Session.cpp#L948 >> to not throw but to simply reject the message). >> >> I will also look at this, though I think the message should be "returned" > rather than "rejected" > > It only just occurred to me that the reason there are items still pending > is probably due to flow control - I had been thinking it was a data race - > so I'll need to keep that in mind when attempting to return a status to the > sender. > > Thanks again > > Pete > > >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: users-unsubscr...@qpid.apache.org >> For additional commands, e-mail: users-h...@qpid.apache.org >> >>
