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

Use Docker time limit for max lock age #4747

Merged
merged 2 commits into from Oct 12, 2018
Merged

Conversation

@humitos
Copy link
Member

@humitos humitos commented Oct 11, 2018

When building a project, if it tooks more than REPO_LOCK_SECONDS and while building after that time another build is triggered for the same Version and the same builder takes the task the lock will be considered "old" and remove and taken by the new build.

This will end up in a collision when accessing the files and it could raise an exception like IOError: [Errno 26] Text file busy. Also, it could fail with another unexpected reasons.

This PR increases the max_lock_age to the same value assigned for the project to end the build in order:

  • custom container time limit or,
  • settings.DOCKER_LIMITS['time'] or,
  • settings.REPO_LOCK_SECONDS or,
  • 30 seconds

Related to #1609

@humitos humitos requested a review from Oct 11, 2018
Copy link
Member

@ericholscher ericholscher left a comment

Looks good! (Pending tests)

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 11, 2018

I was looking into this recently and I had the same doubts, the timing didn't seem right, it only prevents users to trigger multiple builds of the same version at the same time (as the lock if release in only 30 seconds). I like more this timing. We may have to considerer users that push to master fix after fix (maybe trying to fix the build time or a typo), those users will get an error. I think this error

https://github.com/rtfd/readthedocs.org/blob/c01e8eb15e6c630e6f999bcd46cd78cb9109141c/readthedocs/doc_builder/exceptions.py#L36-L39

We should change the message (or, if I read correctly, is rtd the one who would retry to build again?)

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Oct 11, 2018

When building a project, if it tooks more than `REPO_LOCK_SECONDS` and
while building after that time another build is triggered for the same
Version and the same builder takes the task the lock will be
considered "old" and remove and taken by the new build.

This will end up in a collision when accessing the files and it could
raise an exception like `IOError: [Errno 26] Text file busy`. Also, it
could fail with another unexpected reasons.

This PR increases the `max_lock_age` to the same value assigned for
the project to end the build in order:

* custom container time limit or,
* `settings.DOCKER_LIMITS['time']` or,
* `settings.REPO_LOCK_SECONDS` or,
* 30 seconds

Related to #1609
@humitos humitos force-pushed the humitos/build/lock-timeout branch from b07c3a3 to a6c2481 Oct 11, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Oct 11, 2018

as the lock if release in only 30 seconds

In production is 300, by the way. This setting is override.

@humitos
Copy link
Member Author

@humitos humitos commented Oct 11, 2018

@stsewd are you OK by merging this PR or do you think there is something else missing?

return NonBlockingLock(project=self, version=version, max_lock_age=max_lock_age)
def repo_nonblockinglock(self, version, max_lock_age=None):
"""
Return a ``NonBlockingLock`` or raise an exception.
Copy link
Member

@stsewd stsewd Oct 11, 2018

I don't think we raise an exception here, it comes from the context manager.

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 11, 2018

I think we are good, I just left a comment on the docstring

@humitos humitos merged commit cd994c2 into master Oct 12, 2018
1 check passed
@humitos humitos deleted the humitos/build/lock-timeout branch Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants