Skip to content

Use lock when popping requests on server shutdown#13422

Merged
kpayson64 merged 1 commit intogrpc:masterfrom
kpayson64:quic_tsan_fix
Nov 17, 2017
Merged

Use lock when popping requests on server shutdown#13422
kpayson64 merged 1 commit intogrpc:masterfrom
kpayson64:quic_tsan_fix

Conversation

@kpayson64
Copy link
Copy Markdown
Contributor

@kpayson64 kpayson64 commented Nov 16, 2017

Doing this without a lock causes TSAN failures for quic.

There isn't much need to be clever here because this only impacts
shutdown performance, which doesn't really matter.

@kpayson64
Use lock when popping requests on server shutdown
24908a6 
Doing this without a lock causes TSAN failures for quic.

There isn't much need to be clever here because this only impacts
shutdown performance, which doesn't really matter.
@kpayson64 kpayson64 requested a review from vjpai November 16, 2017 20:39
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

****************************************************************

libgrpc.so

     VM SIZE              FILE SIZE
 ++++++++++++++ GROWIN ++++++++++++++

 -------------- SHRINK --------------
  [ = ]       0 [None]     -24  -0.0%

  [ = ]       0 TOTAL      -24  -0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[microbenchmarks] No significant performance differences

@vjpai
Copy link
Copy Markdown
Contributor

vjpai commented Nov 16, 2017

I approved, but can you go ahead and add in a comment saying why it's ok to do this under a lock. Any idea why the transport has an impact?

@kpayson64
Copy link
Copy Markdown
Contributor Author

kpayson64 commented Nov 17, 2017

gpr_locked_mpscq_pop() is always safe (and is the default in the request matcher implementation), so I think adding justification about its safety in this spot might be more confusing than helpful.

As to why it fails with the quic transport, I'm not fully sure. The TSAN traces show concurrent pop() operations, so something about the transport is likely violating assumption 2:

2. no other threads are pulling (since the shut down process is single		
 -          threaded)

@kpayson64
Copy link
Copy Markdown
Contributor Author

kpayson64 commented Nov 17, 2017

#13178
#13434
#13426

@kpayson64 kpayson64 merged commit c35a660 into grpc:master Nov 17, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Feb 12, 2021
route config: Allow to configure direct response body size (grpc#14778)
7951082 
This patch adds max_direct_response_body_size_bytes to set the maximum bytes of the direct response body size (in bytes). The config is added as a field in RouteConfiguration.

Reviving grpc#13487 with a slightly different approach (add the config to RouteConfiguration instead of directly per direct response config entry).

Risk Level: Low, since the default behavior is preserved.
Testing: Updated to test the newly introduced config.
Docs Changes: Updated.
Release Notes: Added.
Fixes grpc#13422

Signed-off-by: Dhi Aurrahman <[email protected]>

Mirrored from https://github.com/envoyproxy/envoy @ eeb7adc3a30456f0d4ac65e5e6c8e88e25481d2a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@vjpai vjpai vjpai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vjpai vjpai

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kpayson64 @grpc-testing @vjpai