Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

[PDS-10{3841,4895}] Part 5: Don't Reuse Streams In The Stream Store #4444

Merged
merged 13 commits into from
Nov 26, 2019

Conversation

jeremyk-91
Copy link
Contributor

Thanks to @jackwickham and @j-baker for helping to puzzle through this issue!

Goals (and why):

  • Don't break users who use storeStreams plural, and read the Sha256Hash data from the stream for whatever reason. Previously, this would invoke storeBlocksAndGetHashlessMetadata multiple times. But since the input stream was already consumed, it either returns nothing (e.g. if using ByteArrayInputStream), or throws.

Implementation Description (bullets):

  • Store streams and generate metadata eagerly.

Testing (What was existing testing like? What have you done to improve it?):
Added two new tests validating

  • streams are not used once closed
  • that reading the metadata is allowed and won't cause SEVERE DATA CORRUPTION™️.

Concerns (what feedback would you like?):

  • General correctness. Nothing too special.

Where should we start reviewing?: StreamTest

Priority (whenever / two weeks / yesterday): yesterday

@changelog-app
Copy link

changelog-app bot commented Nov 26, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

We no longer read and physically store streams lazily in the AtlasDB store when transactionally storing a stream (via storeStreams()).

Previously, users who stored and marked streams in a single transaction and subsequently referenced the returned Sha256Hash may fail in the following ways:

  • if the provided InputStreams throw when they are read after being closed, these exceptions would be thrown out to the user.
  • if the provided InputStreams do not throw when they are read after being closed, AtlasDB would erroneously write an empty block as the first block of the stream, and incorrect metadata of the stream being an empty stream.

Check the box to generate changelog(s)

  • Generate changelog entry

@jeremyk-91
Copy link
Contributor Author

(This is an unforked version of #4443 plus tests)

Copy link
Contributor

@OStevan OStevan left a comment

Choose a reason for hiding this comment

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

Thanks for asking me to review.

Copy link
Contributor

@OStevan OStevan left a comment

Choose a reason for hiding this comment

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

Looks good.

@jeremyk-91
Copy link
Contributor Author

Thanks for the reviews!

@jeremyk-91 jeremyk-91 merged commit b3c12ec into develop Nov 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the fire/stream-store-sadness branch November 26, 2019 20:55
@svc-autorelease
Copy link
Collaborator

Released 0.174.2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants