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

receive: ensure callback errors are propagated #195

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented Apr 3, 2024

Previously, it was possible for errors returned by an asyncronous callback function to not be propagated, and to instead return "context canceled" from Receive.

This could happen based on this specific sequence of events:

  • An asyncronous callback is called from HandleChange, and returns an error, completing the async errorgroup context with an error state.
  • Another call to HandleChange completes, and uses the context syncronously, returning context.Canceled.
  • This in turn propagates to doubleWalkDiff, which returns the canceled error, instead of calling Wait which would contain the actual error group error.

This behavior is racy, based on exactly how and when the asyncronous callback is called.

The fix to this is relatively straightforward: we just need to make sure that syncronous and asyncronous function calls are kept separate, and don't reuse the same context. This means that a failure in an async callback, doesn't cause later sync ones to fail.

Previously, it was possible for errors returned by an asyncronous
callback function to not be propagated, and to instead return "context
canceled" from `Receive`.

This could happen based on this specific sequence of events:
- An asyncronous callback is called from `HandleChange`, and returns an
  error, completing the async errorgroup context with an error state.
- Another call to `HandleChange` completes, and uses the context
  syncronously, returning `context.Canceled`.
- This in turn propagates to `doubleWalkDiff`, which returns the
  canceled error, instead of calling `Wait` which would contain the
  actual error group error.

This behavior is racy, based on exactly how and when the asyncronous
callback is called.

The fix to this is relatively straightforward: we just need to make sure
that syncronous and asyncronous function calls are kept separate, and
don't reuse the same context. This means that a failure in an async
callback, doesn't cause later sync ones to fail.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
Copy link
Collaborator Author

jedevc commented Apr 3, 2024

If you're interested in reproing, you can get this to happen by inserting a fake error return into receiver.asyncDataFunc, and then running go test -v -count=100 -run TestCopySimple ./... - some of the failures will include this error, while others don't propagate it correctly.

@tonistiigi tonistiigi merged commit 9c5337f into tonistiigi:master Apr 15, 2024
14 checks passed
profnandaa added a commit to profnandaa/buildkit that referenced this pull request Apr 22, 2024
Full diff: https://github.com/tonistiigi/fsutil/compare/7525a1af2bb5..497d33b

Summary changes:
- tonistiigi/fsutil#195
  receive: ensure callback errors are propagated
- tonistiigi/fsutil#196
  Fix file transfers from windows to linux
  fixes moby#4741
- tonistiigi/fsutil#197
  recv: translate linkname to wire format

Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
profnandaa added a commit to profnandaa/buildkit that referenced this pull request Apr 22, 2024
Full diff: https://github.com/tonistiigi/fsutil/compare/7525a1af2bb5..497d33b

Summary changes:
- tonistiigi/fsutil#195
  receive: ensure callback errors are propagated
- tonistiigi/fsutil#196
  Fix file transfers from windows to linux
  fixes moby#4741
- tonistiigi/fsutil#197
  recv: translate linkname to wire format

Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
profnandaa added a commit to profnandaa/buildkit that referenced this pull request Apr 22, 2024
Full diff: https://github.com/tonistiigi/fsutil/compare/7525a1af2bb5..497d33b

Summary changes:
- tonistiigi/fsutil#195
  receive: ensure callback errors are propagated
- tonistiigi/fsutil#196
  Fix file transfers from windows to linux
  fixes moby#4741
- tonistiigi/fsutil#197
  recv: translate linkname to wire format

Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
tonistiigi pushed a commit to tonistiigi/buildkit that referenced this pull request Apr 24, 2024
Full diff: https://github.com/tonistiigi/fsutil/compare/7525a1af2bb5..497d33b

Summary changes:
- tonistiigi/fsutil#195
  receive: ensure callback errors are propagated
- tonistiigi/fsutil#196
  Fix file transfers from windows to linux
  fixes moby#4741
- tonistiigi/fsutil#197
  recv: translate linkname to wire format

Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
(cherry picked from commit 64ea9da)
tonistiigi pushed a commit to tonistiigi/buildkit that referenced this pull request Apr 25, 2024
Full diff: https://github.com/tonistiigi/fsutil/compare/7525a1af2bb5..497d33b

Summary changes:
- tonistiigi/fsutil#195
  receive: ensure callback errors are propagated
- tonistiigi/fsutil#196
  Fix file transfers from windows to linux
  fixes moby#4741
- tonistiigi/fsutil#197
  recv: translate linkname to wire format

Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
(cherry picked from commit 64ea9da)
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

2 participants