coapjs / node-coap

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

Improve close logic and fix corner cases #329

Closed relu91 closed 2 years ago

relu91 commented 2 years ago

Hello, this is my first PR to this repository let me know if I have to change the commit style or if I skipped some steps. Regarding the PR I'm coming from https://github.com/eclipse/thingweb.node-wot/issues/637 where we found that node hanged after using coap client. I've dug into the code and I found out that the problem arises when you close an agent when there are no pending requests running. Basically if you take a look at the current implementation code:

 close (done?: (err?: Error) => void): this {
        for (const req of this._msgIdToReq.values()) {
            this.abort(req) // <-- never called if this._msgIdToReq.size === 0
        }
        if (done != null) {
            setImmediate(done)
        }
        return this
    }

The abort function is never called which in the end has the effect of not properly closing the socket. Net result? node hangs and does not exit.

This PR fix this shortcoming and a couple of other details:

  1. done function is only called after the socket is really closed
  2. It adds a listener for errors in a test (on my machine whiteout that listener the test suite does not continue)

Warning after a clean clone on windows with node 14.17.0 I have a couple of tests that are still failing, this PR does not fix those.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1991863206


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/agent.ts 11 14 78.57%
<!-- Total: 11 14 78.57% -->
Files with Coverage Reduction New Missed Lines %
lib/agent.ts 1 83.18%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 1739754569: -0.2%
Covered Lines: 1142
Relevant Lines: 1258

💛 - Coveralls