Fix for max_concurrent_streams issue - Call mark_stream_closed before… #12463

yashykt · 2017-09-08T22:30:48Z

Fix for max_concurrent_streams issue - Call mark_stream_closed before…

yashykt · 2017-09-08T23:57:32Z

READY FOR REVIEW

grpc-kokoro · 2017-09-09T01:02:03Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_256k_256k_4M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_16M_16M_4M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_256k_256k_64M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_256k_256k_256k.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_256k_64M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_256k_4M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_16M_4M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_16M_16M_256k.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_16M_256k.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_256k_16k.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_256k_256k_16k.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_16M_64M.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_1_256k_256k.opt.new: 4
    bm_fullstack_trickle.BM_PumpUnbalancedUnary_Trickle_16M_16M_64M.opt.new: 4


[trickle] No significant performance differences

grpc-kokoro · 2017-09-09T02:21:12Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-09T02:46:57Z

[microbenchmarks] Performance differences noted:
Benchmark                                                    cpu_time    real_time
-----------------------------------------------------------  ----------  -----------
BM_HpackParserParseHeader<NonIndexedBinaryElem<100, false>>  +4%         +5%

grpc-kokoro · 2017-09-10T09:28:59Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-10T10:04:59Z

[microbenchmarks] No significant performance differences

yashykt · 2017-09-11T17:38:14Z

Known Issues
Basic Tests C & C++ Linux [opt] : #11737
Basic Tests Linux (opt) : #11737
Cloud To Prod Interop : #10763
Msan C (Internal CI) : #12307
UBsan C (Internal CI) : #11528

grpc-kokoro · 2017-09-11T18:09:46Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-11T18:50:56Z

[microbenchmarks] No significant performance differences

ncteisen

I started reviewing then realized I had the same question each time.

mark_stream_closed calls fail_pending_writes. Is the race caused because fail_pending_writes needs to be called earlier? In that case could we just change mark_stream_closed?

It seems a little funny that we have to duplicate some checks, call fail_pending_writes, which is then called again from inside mark_stream_closed

ncteisen · 2017-09-11T21:25:56Z

src/core/ext/transport/chttp2/transport/hpack_parser.c

@@ -1650,6 +1650,7 @@ static void force_client_rst_stream(grpc_exec_ctx *exec_ctx, void *sp,
        &t->qbuf, grpc_chttp2_rst_stream_create(s->id, GRPC_HTTP2_NO_ERROR,
                                                &s->stats.outgoing));
    grpc_chttp2_initiate_write(exec_ctx, t, "force_rst_stream");
+    grpc_chttp2_fail_pending_writes(exec_ctx, t, s, GRPC_ERROR_NONE);


Will this not be invoked from mark_stream_closed?

The main issue is not with fail_pending_writes, but with a race while sending trailing metadata, and actually deleting the stream from the stream_map. The stream_map size is currently used to check the number of streams in use currently. Earlier, the server would send the trailing metadata and only remove the stream after that, which gave rise to the possibility of the client sending another stream before the original was removed, creating the flake that we see.

The solution was to call mark_stream_closed before sending the data. To do this, I had to remove fail_pending_writes (from mark_stream_closed) which was causing a few issues.
(I actually got confused when I saw your comment. I thought maybe that I had left removing the line from mark_stream_closed, but then I realized that your link points to the current grpc master, and not the pull request :))

[accidentally wrote and delete comment. Re writing]

Oops, I was half reviewing your code, and half reviewing an old chrome tab open at master. The dangers of too many tabs...

Actually, this just reminded me something. I changed the fix a bit and this was after noticing the fail_pending_writes. The fail_pending_writes change may no longer be required. The last I talked to Craig, as long as something is put on the qbuf, it should go out. Let me try adding the fail_pending_writes back in.

ncteisen · 2017-09-11T21:29:33Z

src/core/ext/transport/chttp2/transport/frame_rst_stream.c

@@ -103,6 +103,9 @@ grpc_error *grpc_chttp2_rst_stream_parser_parse(grpc_exec_ctx *exec_ctx,
          GRPC_ERROR_INT_HTTP2_ERROR, (intptr_t)reason);
      gpr_free(message);
    }
+    if (!s->write_closed) {


Same question. Seems like this will occur from here

ncteisen

Nice work tracking down a difficult, racy bug. LGTM

ncteisen · 2017-09-11T22:09:23Z

src/core/ext/transport/chttp2/transport/hpack_parser.c

@@ -1650,6 +1650,7 @@ static void force_client_rst_stream(grpc_exec_ctx *exec_ctx, void *sp,
        &t->qbuf, grpc_chttp2_rst_stream_create(s->id, GRPC_HTTP2_NO_ERROR,
                                                &s->stats.outgoing));
    grpc_chttp2_initiate_write(exec_ctx, t, "force_rst_stream");
+    grpc_chttp2_fail_pending_writes(exec_ctx, t, s, GRPC_ERROR_NONE);


[accidentally wrote and delete comment. Re writing]

Oops, I was half reviewing your code, and half reviewing an old chrome tab open at master. The dangers of too many tabs...

yashykt · 2017-09-11T22:53:16Z

The change is much cleaner and smaller now. Earlier, I had added the mark_stream_closed much earlier than necessary which was causing the fail_pending_writes to result in issues. At the current placement of mark_stream_closed in writing.c, there is no need for changing mark_stream_closed or bring out fail_pending_writes :)

ncteisen · 2017-09-11T23:46:13Z

Much cleaner, nice. re-LGTM

yashykt · 2017-09-12T08:11:14Z

Jenkins: retest this please

grpc-kokoro · 2017-09-12T17:59:20Z

[trickle] No significant performance differences

… sending trailing metadata

…d anymore

grpc-kokoro · 2017-09-13T00:26:33Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-13T00:47:08Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.opt.old: 1
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_InProcessCHTTP2_NoOpMutator_NoOpMutator__2M_1_0.counters.old: 1


[microbenchmarks] No significant performance differences

yashykt · 2017-09-13T05:17:39Z

Known issues : #12527, #11055, #11737, #12307

googlebot added the cla: yes label Sep 8, 2017

yashykt force-pushed the max_concurrent_streams_fix branch from 07b7235 to a6e36e9 Compare September 9, 2017 00:25

yashykt mentioned this pull request Sep 11, 2017

bins/ubsan/h2_proxy_test payload GRPC_POLL_STRATEGY=poll flaky: "value is outside the range of representable values of type 'int'" #11528

Closed

yashykt force-pushed the max_concurrent_streams_fix branch from c1ffbc8 to e45b3ec Compare September 11, 2017 17:40

yashykt requested a review from ncteisen September 11, 2017 18:01

ncteisen self-assigned this Sep 11, 2017

ncteisen reviewed Sep 11, 2017

View reviewed changes

ncteisen approved these changes Sep 11, 2017

View reviewed changes

yashykt added 3 commits September 12, 2017 15:19

Fix for max_concurrent_streams issue - Call mark_stream_closed before…

49119a4

… sending trailing metadata

There is no need to remove fail_pending_writes from mark_stream_close…

6988dbb

…d anymore

Remove unnecessary comment

a6ede3b

yashykt force-pushed the max_concurrent_streams_fix branch from cbc92c7 to a6ede3b Compare September 12, 2017 22:20

yashykt merged commit f21e7ef into grpc:master Sep 13, 2017

yashykt mentioned this pull request Sep 13, 2017

"h2_http_proxy_test max_concurrent_streams" end2end test does not work with epoll1 #12201

Closed

This was referenced Sep 13, 2017

h2_sockpair+trace_test.exe max_concurrent_streams #12435

Closed

c_windows_dbg_native.cmake/build/Debug/h2_sockpair_1byte_nosec_test.exe max_concurrent_streams failure #12388

Closed

yashykt deleted the max_concurrent_streams_fix branch March 14, 2018 22:12

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for max_concurrent_streams issue - Call mark_stream_closed before… #12463

Fix for max_concurrent_streams issue - Call mark_stream_closed before… #12463

yashykt commented Sep 8, 2017

yashykt commented Sep 8, 2017 •

edited

Loading

grpc-kokoro commented Sep 9, 2017

grpc-kokoro commented Sep 9, 2017

grpc-kokoro commented Sep 9, 2017

grpc-kokoro commented Sep 10, 2017

grpc-kokoro commented Sep 10, 2017

yashykt commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

ncteisen left a comment •

edited

Loading

ncteisen Sep 11, 2017

yashykt Sep 11, 2017

ncteisen Sep 11, 2017

yashykt Sep 11, 2017

ncteisen Sep 11, 2017

ncteisen left a comment

ncteisen Sep 11, 2017

yashykt commented Sep 11, 2017

ncteisen commented Sep 11, 2017

yashykt commented Sep 12, 2017 •

edited

Loading

grpc-kokoro commented Sep 12, 2017

grpc-kokoro commented Sep 13, 2017

grpc-kokoro commented Sep 13, 2017

yashykt commented Sep 13, 2017

Fix for max_concurrent_streams issue - Call mark_stream_closed before… #12463

Fix for max_concurrent_streams issue - Call mark_stream_closed before… #12463

Conversation

yashykt commented Sep 8, 2017

yashykt commented Sep 8, 2017 • edited Loading

grpc-kokoro commented Sep 9, 2017

grpc-kokoro commented Sep 9, 2017

grpc-kokoro commented Sep 9, 2017

grpc-kokoro commented Sep 10, 2017

grpc-kokoro commented Sep 10, 2017

yashykt commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

grpc-kokoro commented Sep 11, 2017

ncteisen left a comment • edited Loading

Choose a reason for hiding this comment

ncteisen Sep 11, 2017

Choose a reason for hiding this comment

yashykt Sep 11, 2017

Choose a reason for hiding this comment

ncteisen Sep 11, 2017

Choose a reason for hiding this comment

yashykt Sep 11, 2017

Choose a reason for hiding this comment

ncteisen Sep 11, 2017

Choose a reason for hiding this comment

ncteisen left a comment

Choose a reason for hiding this comment

ncteisen Sep 11, 2017

Choose a reason for hiding this comment

yashykt commented Sep 11, 2017

ncteisen commented Sep 11, 2017

yashykt commented Sep 12, 2017 • edited Loading

grpc-kokoro commented Sep 12, 2017

grpc-kokoro commented Sep 13, 2017

grpc-kokoro commented Sep 13, 2017

yashykt commented Sep 13, 2017

yashykt commented Sep 8, 2017 •

edited

Loading

ncteisen left a comment •

edited

Loading

yashykt commented Sep 12, 2017 •

edited

Loading