go extension trailer add api #27350
go extension trailer add api #27350
Conversation
Signed-off-by: wangkai19 <[email protected]>
|
@doujiang24 Hi, here is the trailer add api for go extension, and i find some bug in trailer phase when i try to add trailer related integration test, but i am not sure if it is fixed correctly. When |
| } | ||
|
|
||
| func (h *requestOrResponseTrailerMapImpl) Add(key, value string) { | ||
| panic("unsupported yet") | ||
| h.initHeaders() |
There was a problem hiding this comment.
| h.initHeaders() | |
| h.initTrailers() |
yep, it should be an old issue, let's fix it here, thanks!
There was a problem hiding this comment.
oh, this means we need a trailerMapImpl like headerMapImpl?
There was a problem hiding this comment.
nope, I don't think so, just update the initHeaders of requestOrResponseTrailerMapImpl could be ok? at line 176.
There was a problem hiding this comment.
done, PTAL again, thanks.
| done = doTrailerGo(state, trailers); | ||
| } | ||
| state.doDataList.moveOut(body); |
There was a problem hiding this comment.
oh, seems it's an old bug?
seems we should add these after line 347? instead of these two line.
if (done) {
state.continueDoData();
}
There was a problem hiding this comment.
emm, line after 347 is not
if (done) {
state.continueDoData();
}
There was a problem hiding this comment.
yep, there is not yet, I mean we should add it there, and we do not these two new lines then.
There was a problem hiding this comment.
emmm, i think moveOut should not be there, since even if isBufferDataEmpty is true, doDataList may be also not empty, for example, decodeHeaders returns continue and decodeData returns StopAndBuffer, doDataGo will be called at decodeData phase first, so doDataList will not be empty at decodeTrailers phase. Did i make myself clear? 😄
There was a problem hiding this comment.
oh, yes, the case you said is right.
but, the current implemention is not a proper fix. for example, doDataGo may return false at line 347, and we should not send doDataList then.
I think the proper fix might be, add continueDoData in the else branch, before doTrailerGo at line 355.
There was a problem hiding this comment.
Also, could you please add some related test case? thanks very much.
Ah, seem this PR is bugfix more than feature, cool!
| // NP: can not use done as condition here, since done will be false | ||
| // maybe we can remove the done variable totally? by using state_ only? | ||
| // continue trailers | ||
| if (state.state() == FilterState::WaitingTrailer) { | ||
| done = doTrailerGo(state, trailers); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I'm not sure about these changes, could you please explain them?
There was a problem hiding this comment.
just like what i comment above, doTrailerGo should also be called when isBufferDataEmpty is true, otherwize the filter chain will be stopped.
There was a problem hiding this comment.
oh, yes, I see, didn't see the comment before I comment, thanks.
Signed-off-by: wangkai19 <[email protected]>
| done = doTrailerGo(state, trailers); | ||
| } | ||
| state.doDataList.moveOut(body); |
There was a problem hiding this comment.
oh, yes, the case you said is right.
but, the current implemention is not a proper fix. for example, doDataGo may return false at line 347, and we should not send doDataList then.
I think the proper fix might be, add continueDoData in the else branch, before doTrailerGo at line 355.
| done = doTrailerGo(state, trailers); | ||
| } | ||
| state.doDataList.moveOut(body); |
There was a problem hiding this comment.
Also, could you please add some related test case? thanks very much.
Ah, seem this PR is bugfix more than feature, cool!
|
/assign @mattklein123 |
Signed-off-by: wangkai19 <[email protected]>
|
@doujiang24 Comments above are resolved as disscusion offline, and some test cases are already available like |
Signed-off-by: wangkai19 <[email protected]> Signed-off-by: Shriram Rajagopalan <[email protected]>
Signed-off-by: wangkai19 <[email protected]> Signed-off-by: Ryan Eskin <[email protected]>
Commit Message: go extension trailer add api
Additional Description: N/A
Risk Level: low
Testing: integration test
Docs Changes: N/A
Release Notes: N/A