Skip to content

Public methods must not panic after Close()#202

Merged
martinmr merged 1 commit intodgraph-io:masterfrom
abliqo:abliqo/panic
Oct 6, 2020
Merged

Public methods must not panic after Close()#202
martinmr merged 1 commit intodgraph-io:masterfrom
abliqo:abliqo/panic

Conversation

@abliqo
Copy link
Contributor

@abliqo abliqo commented Oct 5, 2020 •
edited by manishrjain
Loading

The process crashes when callsites have many goroutines calling public methods after Close() was already called.
That must be handled gracefully.

panic: send on closed channel

goroutine 24430 [running]:
{path_redacted}/vendor/github.com/dgraph-io/ristretto.(*defaultPolicy).Push(0xc000676060, 0xc1417ed200, 0x40, 0x40, 0x3)
	{path_redacted}/vendor/github.com/dgraph-io/ristretto/policy.go:112 +0x64
{path_redacted}/vendor/github.com/dgraph-io/ristretto.(*ringStripe).Push(0xc14a61b2c0, 0xebbce095e9cac6f6)
	{path_redacted}/vendor/github.com/dgraph-io/ristretto/ring.go:51 +0x95
{path_redacted}/vendor/github.com/dgraph-io/ristretto.(*ringBuffer).Push(0xc00000e138, 0xebbce095e9cac6f6)
	{path_redacted}/vendor/github.com/dgraph-io/ristretto/ring.go:89 +0x60
{path_redacted}/vendor/github.com/dgraph-io/ristretto.(*Cache).Get(0xc0004fcf60, 0x14f8320, 0xc155a21d28, 0xbff0000000000000, 0x0, 0x0)
	{path_redacted}/vendor/github.com/dgraph-io/ristretto/cache.go:167 +0x8e

panic: send on closed channel

goroutine 23589 [running]:
{path_redacted}/vendor/github.com/dgraph-io/ristretto.(*Cache).Set(0xc0005ab800, 0x14d46a0, 0xc055784c00, 0x1643ee0, 0xc18eb74300, 0x1, 0xc16250bc00)
	{path_redacted}/vendor/github.com/dgraph-io/ristretto/cache.go:205 +0x12b

This change is Reviewable

@abliqo
Public methods must not panic after Close()
7758801 
The process crashes when callsites have many goroutines calling public methods after `Close()` was already called.
That must be handled gracefully.

```
panic: send on closed channel

goroutine 24430 [running]:
code.uber.internal/infra/statsdex/vendor/github.com/dgraph-io/ristretto.(*defaultPolicy).Push(0xc000676060, 0xc1417ed200, 0x40, 0x40, 0x3)
	code.uber.internal/infra/statsdex/vendor/github.com/dgraph-io/ristretto/policy.go:112 +0x64
```
@CLAassistant
Copy link

CLAassistant commented Oct 5, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@abliqo
Copy link
Contributor Author

abliqo commented Oct 6, 2020

@manishrjain @martinmr, could you please review this? Thanks

martinmr
martinmr approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jarifibrahim and @manishrjain)

@martinmr martinmr merged commit 88ad187 into dgraph-io:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jarifibrahim jarifibrahim Awaiting requested review from jarifibrahim

@manishrjain manishrjain Awaiting requested review from manishrjain

1 more reviewer

@martinmr martinmr martinmr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@abliqo @CLAassistant @martinmr