Skip to content
New issue

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

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

Already on GitHub? Sign in 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants