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

js: unblock batch requests on 408 with at least a message #823

Merged
merged 1 commit into from Sep 16, 2021

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Sep 14, 2021

This makes it so that fetch requests that receive a 408 message signal to be canceled become unblocked in case they have received a single message at least.

Signed-off-by: Waldemar Quevedo wally@synadia.com

@wallyqs wallyqs requested review from kozlovic and derekcollison and removed request for kozlovic September 14, 2021 22:05
@coveralls
Copy link

coveralls commented Sep 14, 2021

Coverage Status

Coverage increased (+0.05%) to 87.275% when pulling 7669068 on js-fetch-batch-408 into 1714547 on main.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposing some changes...

test/js_test.go Outdated Show resolved Hide resolved
test/js_test.go Outdated
expected := "hello"
got := string(msg.Data)
if got != expected {
t.Errorf("Expected: %v, got: %v", expected, got)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have t.Errorf()? Any error that we have is fatal, there is no point continuing the test I think.
If you agree, then this comment applies to all other references of t.Errorf() you have in this test.

test/js_test.go Show resolved Hide resolved
test/js_test.go Show resolved Hide resolved
js.go Show resolved Hide resolved
js.go Outdated Show resolved Hide resolved
@wallyqs wallyqs force-pushed the js-fetch-batch-408 branch 6 times, most recently from a573b72 to 673391b Compare September 15, 2021 22:49
Signed-off-by: Waldemar Quevedo <wally@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wallyqs wallyqs merged commit 9292154 into main Sep 16, 2021
@wallyqs wallyqs deleted the js-fetch-batch-408 branch September 16, 2021 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants