coapjs / node-coap

CoAP - Node.js style
MIT License
531 stars 154 forks source link

reset msg on separated response(observe) with block2 option #336

Closed Thousif-khan closed 2 years ago

Thousif-khan commented 2 years ago

am facing an issue,

  1. node-coap is a client and sends a CON request with observe and block2 option.
  2. serve response with ACK including block2 option with moreFlag set to false.
  3. when the new data is ready at server its send a notification CON message with block2 and moreFlag=true here client sents RESET instead of ACK. Am i doing something wrong here. export
JKRhb commented 2 years ago

Hi @Thousif-khan, thanks for reporting this issue. There is currently a bug regarding observe (#330) that might be related to this problem. A new version with these changes should be released soon (CC @Apollon77), maybe that fixes the issue.

Maybe you could try out the version from my fork already to see if the error disappears? Otherwise some more investigation is probably needed. Another thing you could try out is downgrading node-coap to 0.25.0 (which is the version before the switch to typescript began), this way we could rule out if it is a new bug or an edge case nobody noticed before.

Thousif-khan commented 2 years ago

Hi @JKRhb, i did some testings has you suggested,

  1. Tested with 0.25.0 i was getting ACK and RST later.
  2. I believe have found a bug, please have look at file agent.ts line 333. the request related token is deleted, if block and token is present but not check if observe is in request option.

please suggest.

line

JKRhb commented 2 years ago

2. I believe have found a bug, please have look at file agent.ts line 333. the request related token is deleted, if block and token is present but not check if observe is in request option.

Oh, thank you for testing this out! I guess #330 might also fix this problem, hopefully it will be merged and released soon.

Thousif-khan commented 2 years ago

No , #330 wont fix this issue. we need to add check for observe option present in the req. one way of doing it is else if (block2 != null && packet.token != null && ((0, helpers_1.getOption)(packet.options, 'Observe')) == null ) { // check if no observe option present, then delete it.

Apollon77 commented 2 years ago

Please re-check with v1.0.5 (even if your propoed change is not in there)

Thousif-khan commented 2 years ago

Hi @Apollon77, apologies for the delay, have tested with v1.0.5 and the issue still exists. getting same RST message. please take a look of what i have suggested.

Thousif-khan commented 2 years ago

can you try it out with the same setup has mine.

  1. Coap Client sends request with observe and block2 option.
  2. Coap Server responses with ACK on observe request (No piggy-bagged data here, since separated response model).
  3. Coap Server sends CON message with block_0 and more-flag set. (at this point client should respond with empty ACK message and followed block_1 request, but am seeing RST message).

Reason for RST message should be, no token details found or token is deleted from client.

Thousif-khan commented 2 years ago

One more solution would be https://github.com/mcollina/node-coap/blob/67c12da77a8548cea12f8cea02c5c4809b10e24a/lib/agent.ts#L335 added req.url.observe check } else if (block2 != null && packet.token != null && req.url.observe == null) {

Apollon77 commented 2 years ago

@JKRhb is the pther open PR adressing that maybe?

JKRhb commented 2 years ago

@JKRhb is the pther open PR adressing that maybe?

Oh, yeah, that could actually be the case! I would suggest merging #337 first and then revisiting the issue, trying out @Thousif-khan's approach. Thank you @Thousif-khan for the status updates and the suggestions!

Apollon77 commented 2 years ago

Ok, merged ... so @Thousif-khan net try with githubb version?

Thousif-khan commented 2 years ago

sorry @Apollon77, didn't get you. what should be tried ?

Apollon77 commented 2 years ago

Please install the lib GitHub and check the behavior again. If your issue is not fixed we need to check your proposals from above

JKRhb commented 2 years ago

Please install the lib GitHub and check the behavior again. If your issue is not fixed we need to check your proposals from above

I'm afraid we have a typescript related problem here again :/ #337 should have definitely fixed https://github.com/eclipse/thingweb.node-wot/issues/728, though. Therefore, I think it is reasonable to already publish a new release and revisit this issue afterwards.

Apollon77 commented 2 years ago

@Thousif-khan I will release a 1.0.8 soon which adress other observe issues ... please check if this is aloready enough to also fix your issue ... else we have #344 to try next :-)

Thousif-khan commented 2 years ago

Hi @Apollon77, issue persists. please make a setup has i suggested and test for yourself before next release. .

JKRhb commented 2 years ago

else if (block2 != null && packet.token != null && req.url.observe == null) {

Thanks for the feedback, then we should merge #343 now.