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

Fixes race conditions around task logging #5069

Merged
merged 1 commit into from Sep 18, 2019
Merged

Fixes race conditions around task logging #5069

merged 1 commit into from Sep 18, 2019

Conversation

eatkins
Copy link
Contributor

@eatkins eatkins commented Sep 10, 2019

We have seen failures in scripted to create the output file for streams
and it has also been reported in #5067.
I believe this may caused by the same stream output being initialized by
multiple tasks. To fix this, I add locking on a per-file basis. There
was (and is) additional synchronization on the Streams instance, but
the per-file locks are stored in the Streams companion object so the
locking should be honored no matter which Streams instance calls make.

I also skip the call to IO.touch if the file already exists. I believe
that this is the common case and since IO.touch was being called with
setModified = false, it should be fine to skip the touch when the file
exists.

Prior to this change, I was able to induce the issue in #5067 in roughly
1/50 of calls to scripted actions/cross-multiproject. I wasn't able to
reproduce after this change.

@eed3si9n
Copy link
Member

I believe this may caused by the same stream output being initialized by multiple tasks.

Interesting. I'm probably guilty of reusing streams across tasks without thinking about this.

eed3si9n
eed3si9n previously approved these changes Sep 10, 2019
@eatkins
Copy link
Contributor Author

eatkins commented Sep 10, 2019

I noticed that both in actions/cross-multiproject and the stacktrace reported by @pshirshov in #5067 the failures happened in ivy related tasks. I think a number of these are dynamic tasks so I wonder if that has anything to do with it.

@eatkins
Copy link
Contributor Author

eatkins commented Sep 10, 2019

CI has been failing due to dependency-management/cached-resolution-classifier. I can reproduced on the HEAD of origin/develop. It's confusing though because it seems to be failing to download files from https://oss.sonatype.org/content/repositories/snapshots/com/typesafe/config/0.4.9-SNAPSHOT/ and I am able to download all of the files sbt is failing to download manually.

@eatkins
Copy link
Contributor Author

eatkins commented Sep 10, 2019

I guess this started failing yesterday: https://travis-ci.org/sbt/sbt/builds/582813802

@eed3si9n
Copy link
Member

@eatkins Let's disable dependency-management/cached-resolution-classifier for now.

We have seen failures in scripted to create the output file for streams
and it has also been reported in #5067.
I believe this may caused by the same stream output being initialized by
multiple tasks. To fix this, I add locking on a per-file basis. There
was (and is) additional synchronization on the Streams _instance_, but
the per-file locks are stored in the Streams companion object so the
locking should be honored no matter which Streams instance calls make.

I also skip the call to IO.touch if the file already exists. I believe
that this is the common case and since IO.touch was being called with
setModified = false, it should be fine to skip the touch when the file
exists.

Prior to this change, I was able to induce the issue in #5067 in roughly
1/50 of calls to `scripted actions/cross-multiproject`. I wasn't able to
reproduce after this change.
@eed3si9n eed3si9n merged commit 9d0bb68 into sbt:develop Sep 18, 2019
@eatkins eatkins deleted the stream-locks branch September 19, 2019 15:42
@eed3si9n eed3si9n changed the title Add per file stream locks Fixes race conditions around IO Sep 19, 2019
@eed3si9n eed3si9n added this to the 1.3.1 milestone Sep 19, 2019
@eed3si9n eed3si9n changed the title Fixes race conditions around IO Fixes race conditions around task logging Sep 19, 2019
@eed3si9n eed3si9n mentioned this pull request Sep 19, 2019
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