search

kirm / sip.js

Session Initiation Protocol for node.js
MIT License
431 stars 171 forks source link

Possible design flaw #155

Open MayamaTakeshi opened 2 years ago

MayamaTakeshi commented 2 years ago

I'm load testing sip.js as a UAC in this scenario (sip.js is 127.0.0.1:5040) :


              127.0.0.1:5040                127.0.0.1:5000  │
          ──────────┬─────────          ──────────┬─────────│
  08:21:17.996273   │        INVITE (SDP)         │         │
        +0.000070   │ ──────────────────────────> │         │
  08:21:17.996343   │         100 Trying          │         │
        +0.001146   │ <────────────────────────── │         │
  08:21:17.997489   │  183 Session Progress (SDP) │         │
        +0.000247   │ <────────────────────────── │         │
  08:21:17.997736   │           CANCEL            │         │
        +0.000062   │ ──────────────────────────> │         │
  08:21:17.997798   │           200 OK            │         │
                    │ <────────────────────────── │         │
                    │                             │         │
                    │                             │         │
                    │                             │         │

Notice there is no '487 Request Terminated'. In this case, I can see memory leak as the INVITE transaction is never removed from memory as it doesn't get a final response. So I think the handling of CANCEL should take care of starting a timer for the reception of the final response and if it is not received then generate a local '408 Request Timeout' to release the INVITE transaction from memory.

I tested and confirmed the leak with this gist: https://gist.github.com/MayamaTakeshi/3a163c4ea4a5bf77169b1cd0337340ae

MayamaTakeshi commented 2 years ago

Well, considering that sip.js works purely with transactions, maybe handling of CANCEL transaction should not have any effect in the handling of the INVITE transaction. So maybe what is missing is a way for the user of sip.js to be able to delete an existing transaction.

kirm commented 2 years ago

I added a timer for maximum time transaction can spend in proceeding state. It is here https://github.com/kirm/sip.js/tree/Fix_%23155, please let me know if it resolves the problem.

Also I'm considering a way to add ability to kill a transaction, but that's seems to be a more complex solution than a timer.

MayamaTakeshi commented 2 years ago

Hi, I tested it today with both ringTimeLimit: 15000 and ringTimeLimit: 60000 and the problem doesn't happen anymore as the memory is properly released. Thanks.