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 BackgroundWriteDatabase #4188

Merged
merged 5 commits into from
Nov 28, 2024
Merged

Conversation

tybug
Copy link
Member

@tybug tybug commented Nov 27, 2024

See Zac-HD/hypofuzz#41 (comment); was something like this what you were thinking of?

(I expect coverage to fail here; explicit covering test pending)

@Zac-HD
Copy link
Member

Zac-HD commented Nov 27, 2024

Yep, this is pretty much what I was thinking.

It might be nice to check for nesting somehow, but I'm not sure it's worth the trouble or complications for users. And maybe we should have fuzz_one_input use this?

@tybug
Copy link
Member Author

tybug commented Nov 27, 2024

hmm, without a way to expose db.join, I wonder if adding this to fuzz_one_input is more trouble than it's worth. I did add it and then promptly got test failures (e.g. test_fuzz_one_input[BytesIO]) due to race conditions. If a fuzz_one_input consumer is try/catching exceptions - the only case where background db writes makes a difference - then they are probably interested in analysis of the database...but admittedly are also unlikely to check the db so promptly as to run into this.

@Zac-HD
Copy link
Member

Zac-HD commented Nov 27, 2024

hmm, without a way to expose db.join, I wonder if adding this to fuzz_one_input is more trouble than it's worth. I did add it and then promptly got test failures (e.g. test_fuzz_one_input[BytesIO]) due to race conditions. If a fuzz_one_input consumer is try/catching exceptions - the only case where background db writes makes a difference - then they are probably interested in analysis of the database...but admittedly are also unlikely to check the db so promptly as to run into this.

Eh, fair enough. Let's just suggest in the fuzz_one_input docs that you may wish to use BackgroundWriteDatabase for performance reasons, noting that writes might be delayed (or dropped at shutdown) as a downside.

Comment on lines 700 to 714
def fetch(self, key: bytes) -> Iterable[bytes]:
return self._db.fetch(key)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a convenient way to also retrieve the enqueued-but-not-finalized values? Should we wait for the queue to empty (at least of writes), and then fetch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we don't want to deal with retrieving enqeueued-but-not-finalized values because then we have to replicate the semantics of move + delete, and may start disagreeing with an unsound database which doesn't respect the method contracts. Which wouldn't really be our fault, but still.

I think joining on fetch would be reasonable? (note that the queue is only writes, fetches aren't enqueued). Possibly we want a join: bool = True or timeout: int | None = None parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

"join before fetch" cleans up test usages as well, which makes me more confident this is the correct default behavior.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Nice, yeah, join-on-fetch is clearly the way to go 👍

@Zac-HD Zac-HD merged commit 65bd569 into HypothesisWorks:master Nov 28, 2024
49 checks passed
@tybug tybug deleted the background-db branch November 28, 2024 06:38
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.

2 participants