Fix for messages coming from the message pool always ends up as non-confirmable

[andersroos]

Hello!

While using this lib I discovered that observe requests sometimes wasn't retried properly (here). After a bit of digging I found out that messages coming from the message pool always are set to non-confirmable while new messages not coming from the pool are confirmable by default.

Since the blockwise code copies messages it can end up sending a non-confirmable message when the original message was actually set as confirmable.

I am not very familiar with this code base so I am not sure this fix is good, I have tested it with our server and it seems to solve the issue.

To me it feels wrong to assume what the type should be, so I guess it needs to be set every time a message is acquired? Regardless if it is reused or new? I haven't checked the reset of the code base to see if there are more places like this, just this file. In the meantime I use a message pool with 0 max size as a workaround since most messages should be conformable anyway, but that could cause other issues I suppose, I mean I assume that reset sets a message to non-confirmable for a reason.

Cheers and thanks for a good library!

[Danielius1922]

(Seems OK to me, but I'll wait for Jozef to go over it. He's enjoying his holiday this week, so that'll have to wait for next week.)

[jkralik]

In v2 this is handled by:

go-coap/udp/client/clientconn.go

Line 220 in e59fa91

if req.Options().HasOption(message.Block1) || req.Options().HasOption(message.Block2) {

and

go-coap/udp/client/clientconn.go

Line 296 in e59fa91

return cc.blockWise.WriteMessage(cc.RemoteAddr(), req, cc.blockwiseSZX, cc.session.MaxMessageSize(), func(bwreq blockwise.Message) error {

for v3 this will be refactored to block-wise.

[andersroos]

Hmm, I don't really get this, how is it handled by those lines, can you help me understand? I mean eventually the execution reaches this code

go-coap/udp/client/clientconn.go

Line 233 in 948a0bc

func (cc *ClientConn) writeMessage(req *pool.Message) error {

and the message type is used to determine if there should be retries or not. So unless the type is set explicitly somewhere it will be random, Confirmable if it is a new message created in the message pool or it will be NonConfirmable if it is a reused message from the message pool.

[andersroos]

Thanks @jkralik! Is on a vacation trip at the moment, will take this up again when I am back home.

[jkralik]

@andersroos Pls could you resolve linter issue - https://github.com/plgd-dev/go-coap/actions/runs/3014872294 ? After that I will merge PR.

[andersroos]

@andersroos Pls could you resolve linter issue - https://github.com/plgd-dev/go-coap/actions/runs/3014872294 ? After that I will merge PR.

Sure! Done.

[jkralik]

Great. Thx for contributing !