You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/04/24 06:33:45 UTC
[GitHub] [trafficserver] zhangqiang-01 opened a new pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
zhangqiang-01 opened a new pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#discussion_r416277174
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1907,6 +1907,12 @@ HttpSM::state_read_server_response_header(int event, void *data)
case VC_EVENT_EOS:
server_entry->eos = true;
+ if (vio->ndone == 0) {
Review comment:
This part is probably correct, but there is a similar code below.
https://github.com/apache/trafficserver/blob/e8bb4909bc2c948250ba860a71b8a08f853a4275/proxy/http/HttpSM.cc#L1993-L1994
How about make this parse error and handle it later on the line?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#discussion_r428285958
##########
File path: proxy/hdrs/HdrTSOnly.cc
##########
@@ -59,6 +59,11 @@ HTTPHdr::parse_req(HTTPParser *parser, IOBufferReader *r, int *bytes_used, bool
ParseResult state = PARSE_RESULT_CONT;
*bytes_used = 0;
+ // FIXME: r maybe no has IObufferBlock object
Review comment:
This isn't required because of the `r->block_read_avail()` a bit later. The code for that does the null check on the block -
https://github.com/SolidWallOfCode/trafficserver/blob/master/iocore/eventsystem/P_IOBuffer.h#L493
If it returns a result > 0 then there is guaranteed to be a block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] oknet commented on a change in pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
oknet commented on a change in pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#discussion_r416397260
##########
File path: proxy/http/HttpSM.cc
##########
@@ -1907,6 +1907,12 @@ HttpSM::state_read_server_response_header(int event, void *data)
case VC_EVENT_EOS:
server_entry->eos = true;
+ if (vio->ndone == 0) {
Review comment:
@shinrich removed these code by bba6321e6305f6321ebb5d571c5ad382ab3acf20.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] oknet commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
oknet commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620386576
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zhangqiang-01 edited a comment on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
zhangqiang-01 edited a comment on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620338562
approve， a problem is that IOBuffer was misused，but I don't know how to fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #6707: BugFix: MIOBuffer created in lazy ways, may not has IOBufferBlock
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-769433711
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#discussion_r428285958
##########
File path: proxy/hdrs/HdrTSOnly.cc
##########
@@ -59,6 +59,11 @@ HTTPHdr::parse_req(HTTPParser *parser, IOBufferReader *r, int *bytes_used, bool
ParseResult state = PARSE_RESULT_CONT;
*bytes_used = 0;
+ // FIXME: r maybe no has IObufferBlock object
Review comment:
This isn't required because of the `r->block_read_avail()` a bit later. The code for that does the null check on the block -
https://github.com/SolidWallOfCode/trafficserver/blob/master/iocore/eventsystem/P_IOBuffer.h#L493
Therefore it it returns a result > 0 then there is guaranteed to be a block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zhangqiang-01 commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
zhangqiang-01 commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620338562
approve， a problem is that IOBuffer was misused。
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zhangqiang-01 commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
zhangqiang-01 commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620476906
@maskit
https://github.com/apache/trafficserver/issues/6702
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620331486
The case is not handled appropriately indeed, but I'm not sure if we can always handle it as parse error. It could be the last call which doesn't have any data but with EOF flag on. In that case, PARSE_RESULT_DONE would be appropriate. Maybe, we should let HTTPParser check it.
Also, please run `make clang-format` to pass the CI check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#discussion_r428285958
##########
File path: proxy/hdrs/HdrTSOnly.cc
##########
@@ -59,6 +59,11 @@ HTTPHdr::parse_req(HTTPParser *parser, IOBufferReader *r, int *bytes_used, bool
ParseResult state = PARSE_RESULT_CONT;
*bytes_used = 0;
+ // FIXME: r maybe no has IObufferBlock object
Review comment:
This isn't required because of the `r->block_read_avail()` a bit later. The code for that does the null check on the block -
https://github.com/SolidWallOfCode/trafficserver/blob/master/iocore/eventsystem/P_IOBuffer.h#L493
If it returns a result > 0 there is guaranteed to be a block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#discussion_r428285958
##########
File path: proxy/hdrs/HdrTSOnly.cc
##########
@@ -59,6 +59,11 @@ HTTPHdr::parse_req(HTTPParser *parser, IOBufferReader *r, int *bytes_used, bool
ParseResult state = PARSE_RESULT_CONT;
*bytes_used = 0;
+ // FIXME: r maybe no has IObufferBlock object
Review comment:
This isn't required because of the `r->block_read_avail()` a bit later. The code for that does the null check on the
block -
https://github.com/SolidWallOfCode/trafficserver/blob/master/iocore/eventsystem/P_IOBuffer.h#L493
Therefore it it returns a result > 0 then there is guaranteed to be a block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zhangqiang-01 commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
zhangqiang-01 commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620959943
Thanks, all right。
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zhangqiang-01 edited a comment on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
zhangqiang-01 edited a comment on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620959943
Thanks, all right. Have i close this pr?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall closed pull request #6707: BugFix: MIOBuffer created in lazy ways, may not has IOBufferBlock
Posted by GitBox <gi...@apache.org>.
bryancall closed pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620927032
Thanks. The backtrace is helpful to understand the issue. It seems like master already has a fix for the case. See #3943. Can you try it out?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #6707: BugFix: MIOBuffer created in lazy ways, may not has IOBufferBlock
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-769434068
If this is still an issue please reopen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-620444096
> a problem is that IOBuffer was misused
Can you tell me the detail on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #6707: BugFix: MIOBuffer created in lazzy ways, may not has IOBufferBlock #6702
Posted by GitBox <gi...@apache.org>.
randall commented on pull request #6707:
URL: https://github.com/apache/trafficserver/pull/6707#issuecomment-619056164
[approve ci]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org