Open Read Seekability #15032

seanmcc-msft · 2020-09-09T20:12:10Z

Related #9607

kasobol-msft · 2020-09-10T02:50:35Z

sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs

+            if (newPosition > _length)
+            {
+                if (_allowBlobModifications)
+                {
+                    _position = newPosition;
+                    _seeked = true;
+                    return newPosition;
+                }


Should we check if length has been updated and validate it? What's the scenario where user would like to seek beyond length ?

See my comment below, I'm not sure we can do network calls within Seek().

kasobol-msft · 2020-09-10T02:51:27Z

sdk/storage/Azure.Storage.Blobs/tests/BlobBaseClientTests.cs

+
+            // Act
+            Stream outputStream = await blob.OpenReadAsync(options: options).ConfigureAwait(false);
+            outputStream.Seek(size + 10, SeekOrigin.Begin);


What happens if user tries to read after this operation and blob is not modified ?

An exception would have been thrown within Seek()

kasobol-msft · 2020-09-10T02:53:10Z

sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs

+                case SeekOrigin.End:
+                    if (_allowBlobModifications)
+                    {
+                        throw new ArgumentException($"Cannot {nameof(Seek)} with {nameof(SeekOrigin)}.{nameof(SeekOrigin.End)} on a growing blob or file.");


or we could use current length to calculate this (extra get properties call).

I'm not sure if we can, Seek() is a sync operation, and there doesn't appear to be a SeekAsync() - https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.seek?view=netcore-3.1.

Based on this assumption, I only updated the positions in the Seek() call, and do the necessary network calls in ReadInternal().

makes sense. maybe we should disallow all seeks if blob is modifiable.

seanmcc-msft · 2020-09-11T16:19:10Z

@tg-msft, what do you think?

seanmcc-msft · 2020-09-11T16:24:06Z

Do we need to add seekability tests for SMB Files and Data Lake? This change is in Common, so I was thinking just the Blob tests should be able to cover it.

tg-msft

We should add similar tests for every type of resource that works with OpenReadStream in case there are any service semantics that would cause problems with the behavioral assumptions we're making when we wrap them by streams. And that's more for the sake of future authors who will need to deal with supporting new service features when they're added.

tg-msft · 2020-09-18T21:12:12Z

sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs

+        /// <summary>
+        /// Indicated the user has called Seek() since the last Read() call, and the new position is outside _buffer.
+        /// </summary>
+        private bool _seeked;


Nit - _seeked is kind of a funky name that describes more about the history of actions rather than the current state of the object. What about a name like _bufferInvalidated to make code like if (_bufferPosition == 0 || _bufferPosition == _bufferLength || _bufferInvalidated) read a little easier?

tg-msft · 2020-09-18T21:24:13Z

sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs

-            if (_bufferPosition == 0 || _bufferPosition == _bufferLength)
+            if (_bufferPosition == 0 || _bufferPosition == _bufferLength || _seeked)
            {
                int lastDownloadedBytes = await DownloadInternal(async, cancellationToken).ConfigureAwait(false);
                if (lastDownloadedBytes == 0)
                {
                    return 0;
                }
+                _seeked = false;


In the code 10 lines above this, do you need to change all the condition checks from _position == _length to _position >= _length? What happens in the case where I allow modifications, seek past the end of the blob, and then try to read again? If we update the length above we'll return 0 bytes read. If you don't update the length, you'll drop into DownloadInternal and that'll ask for a range beyond the end of the blob which will cause an exception.

We should also add more test cases for allowBlobModifications:

Seek Beyond End and Find Nothing
stream.Seek(stream.Length + 100); byte[] data = new byte[1]; Assert.Zero(stream.Read(data, 0, 1));

Seek Beyond End and Find Data
stream.Seek(stream.Length + 100); underlyingBlob.Upload(longerDocument); byte[] data = new byte[1]; Assert.NotZero(stream.Read(data, 0, 1));

Did you fix this? The code on line 120/126 is still just checking if (_position == _length). I'm talking about this feedback in particular:

In the code 10 lines above this, do you need to change all the condition checks from _position == _length to _position >= _length? What happens in the case where I allow modifications, seek past the end of the blob, and then try to read again? If we update the length above we'll return 0 bytes read. If you don't update the length, you'll drop into DownloadInternal and that'll ask for a range beyond the end of the blob which will cause an exception.

Unless I was mistaken about the need for a fix here because the extra unit tests showed no problems?

The new tests pass as expected. I get 0 bytes read if there is no data, and the data I was expecting when it exists.

Hmm. I was worried about a 416 per https://docs.microsoft.com/en-us/rest/api/storageservices/specifying-the-range-header-for-blob-service-operations#format-2-bytesstartbyte-endbyte if we sent an invalid range. Is there any way that could get triggered here?

There's a bug in OpenReadAsync_Seek_NewPositionGreaterThanBlobLength_AllowBlobModificationsTrue_NoData test. After fixing it's going to detect it (I left a comment next to it).

tg-msft · 2020-09-18T21:47:07Z

sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs

+                case SeekOrigin.End:
+                    if (_allowBlobModifications)
+                    {
+                        throw new ArgumentException($"Cannot {nameof(Seek)} with {nameof(SeekOrigin)}.{nameof(SeekOrigin.End)} on a growing blob or file.");


We should update the error message to suggest they call Stream.Seek(Stream.Length, SeekOrigin.Begin) if they're trying to get the end of the known data.

tg-msft · 2020-09-18T21:53:33Z

sdk/storage/Azure.Storage.Common/src/Shared/LazyLoadingReadOnlyStream.cs

+                else
+                {
+                    throw new ArgumentException(
+                        $"{nameof(newPosition)} exceeds known blob or file length.  This condition is not allowed with allowBlobModifications == false.");


allowBlobModifications doesn't make much sense when you're working with a generic System.IO.Stream if this error gets raised randomly in the guts of some utility code. Maybe we could say something like You cannot seek past the end of a stream created by OpenReadStream that does not allow modifications to the underlying blob or file.

seanmcc-msft · 2020-09-23T03:17:16Z

@tg-msft, I've created tests for Data Lake and File, and addressed your other comments. Anything else?

tg-msft · 2020-09-24T00:38:26Z

sdk/storage/Azure.Storage.Blobs/tests/BlobBaseClientTests.cs

+            BlobOpenReadOptions options = new BlobOpenReadOptions(allowModifications: true);
+
+            // Act
+            Stream outputStream = await blob.OpenReadAsync(options: options).ConfigureAwait(false);


If you don't read anything, does this set the length before you seek?

Yes, we do a Get Properties request to make sure the blob exists and get it's initial length.

kasobol-msft · 2020-09-29T15:46:32Z

sdk/storage/Azure.Storage.Blobs/tests/BlobBaseClientTests.cs

+            byte[] outputArray = new byte[1];
+
+            // Assert
+            Assert.Zero(stream.Read(outputArray, 0, 1));


We should try to read from stream returned from OpenRead instead of original memory stream. When I try that it throws 416.

Suggested change

Assert.Zero(stream.Read(outputArray, 0, 1));

Assert.Zero(outputStream .Read(outputArray, 0, 1));

You're right. How do you think we should handle this case? We could catch the exception and return 0, maybe there is a better option.

I think @tg-msft suggested to return 0.
One thing to check: what happens if we seek almost till the real end and provide buffer that would result in range spaning across the end, i.e.:

create blob with length 1024

open read and seek till 1100

append 100 bytes to blob

read providing buffer that has 100 bytes+
I wonder if it would throw 412 too in that case, but it should rather read from 1100 till 1024 and return 24.

PaulVrugt · 2020-10-07T20:00:19Z

It would be great if this was merged and released soon. We've been waiting for for this feature patiently for a long time now

seanmcc-msft · 2020-10-07T20:04:02Z

@PaulVrugt, I need to do one more iteration to address seeking to a position > last known blob/file length, and we should be ready to merge.

PaulVrugt · 2020-10-07T20:07:10Z

Thanks Sean, I really appreciate the communication on this. Not so often you see such openness. Others could take an example from you

…known blob/file length

seanmcc-msft · 2020-10-07T21:00:19Z

TODO - update change logs.

seanmcc-msft added 3 commits September 9, 2020 13:48

Inital implementation of OpenRead with seekability

25bc8d9

Added unit tests

fbe4938

Recorded unit tests

e00999d

ghost added the Storage Storage Service (Queues, Blobs, Files) label Sep 9, 2020

seanmcc-msft marked this pull request as ready for review September 10, 2020 00:26

seanmcc-msft requested review from amishra-dev, amnguye, kasobol-msft and tg-msft as code owners September 10, 2020 00:26

kasobol-msft approved these changes Sep 10, 2020

View reviewed changes

amnguye approved these changes Sep 10, 2020

View reviewed changes

seanmcc-msft mentioned this pull request Sep 11, 2020

[QUERY] Missing OpenRead/OpenWrite from Clients #9607

Closed

tg-msft approved these changes Sep 18, 2020

View reviewed changes

seanmcc-msft added 7 commits September 22, 2020 12:08

Merge branch 'master' into feature/storage/openReadSeekability

d75829b

Changed error message

8e5615f

renamed variable

0c6fbc0

Added new live tests

14fa3c1

Updated error message

4de36f1

Unit tests for DataLakeFileClient.OpenRead Seek

e4321d5

Recorded unit tests for file

3c45beb

tg-msft reviewed Sep 24, 2020

View reviewed changes

kasobol-msft reviewed Sep 29, 2020

View reviewed changes

seanmcc-msft added 2 commits October 7, 2020 14:29

Merge branch 'master' into feature/storage/openReadSeekability

4ef8427

hopefully fixed CI

0f015d9

Will always throw and exception if user attempts to seek beyond last …

54e8c1a

…known blob/file length

kasobol-msft approved these changes Oct 7, 2020

View reviewed changes

Updated changelogs

6ad27a7

seanmcc-msft merged commit bdad406 into Azure:master Oct 7, 2020

seanmcc-msft deleted the feature/storage/openReadSeekability branch October 7, 2020 23:42

suhas92 pushed a commit to suhas92/azure-sdk-for-net that referenced this pull request Oct 12, 2020

Open Read Seekability (Azure#15032)

134f254

twsouthwick mentioned this pull request Oct 16, 2020

LazyLoadingReadOnlyStream.Position setter not implemented while Seek is #16055

Closed

annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021

Open Read Seekability (Azure#15032)

0a40a6a

ntsim mentioned this pull request Feb 1, 2023

EES-4074, EES-4017 Update BlobStorageService.StreamBlob to use Azure.Storage.Blobs v12 dfe-analytical-services/explore-education-statistics#3863

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Read Seekability #15032

Open Read Seekability #15032

seanmcc-msft commented Sep 9, 2020

kasobol-msft Sep 10, 2020

seanmcc-msft Sep 10, 2020

kasobol-msft Sep 10, 2020

seanmcc-msft Sep 10, 2020

kasobol-msft Sep 10, 2020

seanmcc-msft Sep 10, 2020

kasobol-msft Sep 10, 2020

seanmcc-msft commented Sep 11, 2020

seanmcc-msft commented Sep 11, 2020

tg-msft left a comment

tg-msft Sep 18, 2020

seanmcc-msft Sep 23, 2020

tg-msft Sep 18, 2020

seanmcc-msft Sep 23, 2020

tg-msft Sep 24, 2020

seanmcc-msft Sep 24, 2020

tg-msft Sep 24, 2020 •

edited

Loading

kasobol-msft Sep 29, 2020

tg-msft Sep 18, 2020

seanmcc-msft Sep 23, 2020

tg-msft Sep 18, 2020

seanmcc-msft Sep 23, 2020

seanmcc-msft commented Sep 23, 2020

tg-msft Sep 24, 2020 •

edited

Loading

seanmcc-msft Sep 29, 2020

kasobol-msft Sep 29, 2020

seanmcc-msft Sep 29, 2020

kasobol-msft Sep 29, 2020 •

edited

Loading

PaulVrugt commented Oct 7, 2020

seanmcc-msft commented Oct 7, 2020

PaulVrugt commented Oct 7, 2020

seanmcc-msft commented Oct 7, 2020

	Assert.Zero(stream.Read(outputArray, 0, 1));
	Assert.Zero(outputStream .Read(outputArray, 0, 1));

Open Read Seekability #15032

Open Read Seekability #15032

Conversation

seanmcc-msft commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft commented Sep 11, 2020

seanmcc-msft commented Sep 11, 2020

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tg-msft Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft commented Sep 23, 2020

tg-msft Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasobol-msft Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

PaulVrugt commented Oct 7, 2020

seanmcc-msft commented Oct 7, 2020

PaulVrugt commented Oct 7, 2020

seanmcc-msft commented Oct 7, 2020

tg-msft Sep 24, 2020 •

edited

Loading

tg-msft Sep 24, 2020 •

edited

Loading

kasobol-msft Sep 29, 2020 •

edited

Loading