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

Update object Put to avoid loosing last chunk #995

Merged
merged 1 commit into from Jun 9, 2022
Merged

Update object Put to avoid loosing last chunk #995

merged 1 commit into from Jun 9, 2022

Conversation

tinou98
Copy link
Contributor

@tinou98 tinou98 commented Jun 7, 2022

The old code might miss the last object if Reader return both a value and EOF

Extract from Reader doc :

When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read. It may return the (non-nil) error from the same call or return the error (and n == 0) from a subsequent call. An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.

Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.

@piotrpio
Copy link
Collaborator

piotrpio commented Jun 8, 2022

Hello @tinou98 , thank you for the contribution! While you're right in that we should process the leftover bytes in case of EOF, I have a couple of comments:

  1. We definitely should not be sending an empty message if we get an EOF error and n == 0 - so this check would have to be added before publishing the message (you can run TestObjectHistory in test/object_test.go to verify).
  2. I think we can safely return immediately if err != nil && err != io.EOF - in that case we don't really care about any processed bytes, as we purge the subject anyway with purgePartial().

After those changes, the flow would look something like this:

  1. Call n, err := r.Read(chunk)
  2. Check if err != nil && err != io.EOF
    • if true, purge and return
  3. Check if n > 0
    • if true, publish message and update counters
  4. Check if err == io.EOF

WDYT?

object.go Outdated Show resolved Hide resolved
@tinou98
Copy link
Contributor Author

tinou98 commented Jun 8, 2022

Updated to match the flow you described.
It also improves the handling of a weird Reader that could return n == 0, but no error.

@coveralls
Copy link

coveralls commented Jun 8, 2022

Coverage Status

Coverage decreased (-0.04%) to 83.915% when pulling 9654d96 on tinou98:patch-1 into 69f0e65 on nats-io:main.

Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison
Copy link
Member

Can we squash this done to one CL?

@tinou98
Copy link
Contributor Author

tinou98 commented Jun 8, 2022

You should be able to Squash & Merge from GitHub UI, otherwise I can Squash & Force Push on my branch

@derekcollison
Copy link
Member

Would you mind squashing and force push?

The old code might miss the last object if `Reader` return both a value and `EOF`

Extract from `Reader` doc :
> When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read. It may return the (non-nil) error from the same call or return the error (and n == 0) from a subsequent call. An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.
>
> Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.
@tinou98
Copy link
Contributor Author

tinou98 commented Jun 9, 2022

Squashed

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution.

@derekcollison derekcollison merged commit 1a55cb9 into nats-io:main Jun 9, 2022
@tinou98 tinou98 deleted the patch-1 branch June 9, 2022 18:08
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.

None yet

4 participants