Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix bug when the value of http header is empty #2888

Merged
merged 2 commits into from
Feb 3, 2022

Conversation

duiniuluantanqin
Copy link
Member

@duiniuluantanqin duiniuluantanqin commented Jan 23, 2022

Summary

Scene: Enable HTTP HOOK
Configuration file:

rtc_server {
    enabled on;
    listen 8000; # UDP port
    # @see https://github.com/ossrs/srs/wiki/v4_CN_WebRTC#config-candidate
    candidate $CANDIDATE;
}

vhost __defaultVhost__ {
    rtc {
        enabled     on;
        # @see https://github.com/ossrs/srs/wiki/v4_CN_WebRTC#rtmp-to-rtc
        rtmp_to_rtc on;
        # @see https://github.com/ossrs/srs/wiki/v4_CN_WebRTC#rtc-to-rtmp
        rtc_to_rtmp off;
    }
    http_remux {
        enabled     on;
        mount       [vhost]/[app]/[stream].flv;
    }
    http_hooks {
        enabled         on;
        on_play         http://192.168.1.10:8888/;
    }
}

Reproduction steps:
To reproduce, use the code provided by @mingyang0921 to set up an HTTP server. The link is here: #2851 (comment)

Cause analysis:
The value of the last field in the HTTP response header is empty, as in the example below, the Server field.

Date: Fri, 31 Dec 2021 08:47:59 GMT\r\nServer: \r\n\r\n{\"code\": 0, \"data\": \"\"}

This will result in the inability to find the body, leading to a timeout in the end. The error log is as follows:

[2022-01-23 15:40:26.982][Warn][41098][78k9o875][62] RTC error code=1011 : RTC: http_hooks_on_play : on_play http://192.168.1.10:8888/ : http: on_play failed, client_id=78k9o875, url=http://192.168.1.10:8888/, request={"server_id":"vid-300d7a8","action":"on_play","client_id":"78k9o875","ip":"192.168.1.6","vhost":"__defaultVhost__","app":"live","stream":"livestream","param":"","pageUrl":""}, response=, status=200 : http: body read : read body : grow buffer : read bytes : timeout 30000 ms
thread [41098][78k9o875]: do_serve_http() [src/app/srs_app_rtc_api.cpp:134][errno=62]
thread [41098][78k9o875]: http_hooks_on_play() [src/app/srs_app_rtc_api.cpp:290][errno=62]
thread [41098][78k9o875]: on_play() [src/app/srs_app_http_hooks.cpp:224][errno=62]
thread [41098][78k9o875]: do_post() [src/app/srs_app_http_hooks.cpp:511][errno=62]
thread [41098][78k9o875]: body_read_all() [src/protocol/srs_service_http_conn.cpp:589][errno=62]
thread [41098][78k9o875]: read_specified() [src/protocol/srs_service_http_conn.cpp:1107][errno=62]
thread [41098][78k9o875]: grow() [src/protocol/srs_protocol_stream.cpp:162][errno=62]
thread [41098][78k9o875]: read() [src/protocol/srs_service_st.cpp:522][errno=62]

Details

Related link: #2851

Others

The http parser upgrade to v2.9.4 still has the same issue, https://github.com/nodejs/http-parser/releases/tag/v2.9.4


TRANS_BY_GPT3

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2022

Codecov Report

Merging #2888 (6ecb3fb) into develop (6b7fc6f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2888      +/-   ##
===========================================
+ Coverage    60.09%   60.10%   +0.01%     
===========================================
  Files          121      121              
  Lines        51312    51326      +14     
===========================================
+ Hits         30836    30850      +14     
  Misses       20476    20476              

| Impacted Files | Coverage Δ | |'

Translated to English while maintaining the markdown structure:

'| Impacted Files | Coverage Δ | |
|---|---|---|
| trunk/src/protocol/srs_service_http_conn.cpp | 94.39% <100.00%> (ø) | |'

Translated to English while maintaining the markdown structure:

| trunk/src/protocol/srs_service_http_conn.cpp | 94.39% <100.00%> (ø) | |
| trunk/src/utest/srs_utest_http.cpp | 99.74% <100.00%> (+<0.01%) | ⬆️ |


Continue to review full report at Codecov.

Legend - Click here to learn more
| Δ = absolute <relative> (impact), ø = not affected, ? = missing data |

Translated to English while maintaining the markdown structure:

| Δ = absolute <relative> (impact), ø = not affected, ? = missing data |

Powered by Codecov. Last update 6b7fc6f...6ecb3fb. Read the comment docs.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 26, 2022

The logic seems very simple, 👍

I remember there is a utest that can cover HTTP parsing. Please check if a new utest case can be added for this scenario.

First of all, thanks to @mingyang0921 for reproducing and submitting the patch.
Thanks to @duiniuluantanqin for improving and finding a better solution.

TRANS_BY_GPT3

@winlinvip winlinvip merged commit b94ce14 into ossrs:develop Feb 3, 2022
winlinvip pushed a commit that referenced this pull request Feb 3, 2022
* Fix bug when the value of http header is empty

* add utest
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants