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.

Sign up for GitHub

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

Jump to bottom

Fix SyncPropagation Defer #4924

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Fix SyncPropagation Defer #4924

merged 1 commit into from
Nov 11, 2024

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Nov 11, 2024

Fixes the defer in the sync propagation. Removes the unnecessary Close call.

@kobergj kobergj requested review from a team, labkode and glpatcern as code owners November 11, 2024 10:15
@micbar
Copy link
Member

micbar commented Nov 11, 2024

@kobergj @aduffeck We have a failing test in https://drone.owncloud.com/cs3org/reva/5521/32/7

@kobergj
Copy link
Contributor Author

kobergj commented Nov 11, 2024 •
edited
Loading

It is different tests failing. Same step (posixfs-integration-tests-4), but different tests. They are flaky. Both were green on the other run.

Run 1
coreApiWebdavDelete/deleteFile.feature:48

Run 2
coreApiTrashbinRestore/trashbinRestore.feature:537

Run 3
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:28
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:29
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:51
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:52
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:72
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:73
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:95
coreApiWebdavEtagPropagation2/restoreFromTrash.feature:96

4 times is a charm ✔️

cc @ScharfViktor @aduffeck

butonic
butonic reviewed Nov 11, 2024
View reviewed changes
pkg/storage/utils/decomposedfs/tree/propagator/sync.go
if err == nil && cerr != nil && !errors.Is(cerr, os.ErrClosed) {
err = cerr // only overwrite err with en error from close if the former was nil
}
_ = os.Remove(f.Name())
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this is the Remove is dropped on purpose, so the inode of the parent lock file remains the same when propagating multiple files concurrently on NFS. This prevents stale NFS file handle errors

@kobergj kobergj merged commit e77dd61 into cs3org:edge Nov 11, 2024
10 checks passed
@kobergj kobergj deleted the FixSyncDefers branch November 11, 2024 16:29
@micbar micbar mentioned this pull request Nov 12, 2024
Open
87 tasks
@butonic butonic mentioned this pull request Nov 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@butonic butonic butonic approved these changes

@labkode labkode Awaiting requested review from labkode labkode is a code owner

@glpatcern glpatcern Awaiting requested review from glpatcern glpatcern is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kobergj @micbar @butonic