sip_transport: tcp: Notify app if unable to keep decoding stream #3961
Conversation
|
|
|
Shouldn't calling the callback be enough? The user will be notified and shutdown the transport if needed. |
|
@trengginas I'm fine with your proposal. TBH I care more about the "logging" side of things to start with, to at least report that the stream fails to parse and becomes broken. Before that, there was no message at all, everything happened silently. I'll submit a new version dropping the transport shutdown. |
| "Shutting down transport %s", | ||
| rdata->tp_info.transport->obj_name)); | ||
|
|
||
| pjsip_transport_shutdown(rdata->tp_info.transport); |
There was a problem hiding this comment.
@trengginas I added the transport_shutdown in the new extended code path because I saw in the other code path it was already being done. So not sure if then this path was wrong then?
Let me know whether you want to merge as it, drop the pjsip_transport_shutdown() from the new path or from both paths.
|
I also vote for callback approach, shutting down transport may not be preferred in some cases so IMO better leave it for app to decide. |
If pjsip_find_msg() starts failing because there's some missing header like Content-Length, log the issue and notify the app about the invalid message. Ideally the app should shutdown the transport to avoid staying in a broken state.
|
I just noticed I actually forgot to update the commit description. I did it now, so I think it's now ready to be merged. |
pespin
changed the title
|
Hmm.. honestly the flow resturcturing make me nervous about possibility of bug or major behavior change. |
|
@nanangizz it's just a matter of looking at pjsip_find_msg() and seeing the error codes returned in each scenario, and act on them. The whole "if (msg_status != PJ_SUCCESS) {" path is transformed in the first case so we get rid of a whole indentation level. Then, the other msg_status are further discriminated. I'm keeping the old code path (the one returning PJSIP_EPARTIALMSG) the same. I'm simply extending the other error codes to handle them, since they were ignored in the past. |
Originally non-success cases are evaluated based on
Indentation is not a very strong reason to modify the flow, the code is only few lines and not that hard to read. CMIIW the new proposed feature in this PR seems to be able to be inserted with much less risk and easier to review :) |
If pjsip_find_msg() starts failing because there's some missing header like Content-Length, log the issue and shutdown the transport to avoid staying in a broken state.