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

Add a test case about "Conform lockingSourceCount is correct" #6490

Closed
wants to merge 3 commits into from

Conversation

QingShiLuoGu
Copy link

@QingShiLuoGu QingShiLuoGu commented Dec 23, 2020

just add a failing test case for #6363 so so you can fix it.

@QingShiLuoGu QingShiLuoGu changed the title Add a test case about "Conform lockingSourceCount is correct #6363" Add a test case about "Conform lockingSourceCount is correct" #6363 Dec 23, 2020
@QingShiLuoGu QingShiLuoGu changed the title Add a test case about "Conform lockingSourceCount is correct" #6363 Add a test case about "Conform lockingSourceCount is correct" Dec 23, 2020
Copy link

@edith2727 edith2727 left a comment

Choose a reason for hiding this comment

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

activar stripe

Copy link

@AFZAL4911 AFZAL4911 left a comment

Choose a reason for hiding this comment

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

Hello dear
How is collect

@yschimke
Copy link
Collaborator

This should now be cleanly reproducible via Okio file system

See

FakeFileSystem().apply { emulateWindows() } to true

Do you want to update the test and I'll take a look?

@QingShiLuoGu
Copy link
Author

hello, do I need fork the latest version and then pull request again?
Or can I just edit this commit? @yschimke

update from latest version
Add a test case about "Conform lockingSourceCount is correct" square#6490
@QingShiLuoGu
Copy link
Author

I think it has been updated,please check it. @yschimke

@yschimke
Copy link
Collaborator

yschimke commented Jul 28, 2021

@swankjesse any opinion on this test?

I'm assuming that DiskLruCache.Snapshot, which is closeable, should have a checkNotClosed() method, and fail fast if you try to use it after closing it. But it breaks a bunch of other tests.

image

Using the snapshot sources will fail as they are closed, but the editor effectively reanimates them.

@swankjesse
Copy link
Collaborator

@yschimke yeah that reads to me like a small bug? The snapshot should not be used after it is closed, though I think there's a valid use case to using a snapshot after it's enclosing cache is closed.

This class isn't public API so we can improve it's behavior where that’s useful. But we don't really need to be super aggressive about enforcing invariants unless we're worried we might inadvertently violate them.

@yschimke
Copy link
Collaborator

Going to close this one out unless we can reproduce with a test case using public API. We can fix this, but it likely just incurs a tiny performance hit on each cache usage.

@yschimke yschimke closed this Jul 29, 2021
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.

5 participants