Skip to content

Lock Improvements for [SR-12851] #130

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

Closed
wants to merge 9 commits into from

Conversation

tgymnich
Copy link
Contributor

@tgymnich tgymnich commented Sep 19, 2020

This PR contains the following improvements to locks needed for SR-12851.

1. Added ReadWriteLock. (Using GCD instead for windows compatiblity)
2. Added property wrapper @ThreadLocal to address an issue where FileLock could be used in way that would unlock/lock the lock from another thread without the user realizing. --> Making FileLock thread safe
3. Enabled FileLock to obtain an exclusive or a shared lock.

@compnerd
Copy link
Member

Please do not merge this without testing on Windows, it will break the s-p-m build. pthread_* functions are definitely not going to work on Windows.

@tgymnich
Copy link
Contributor Author

I am trying to figure the windows thing out right know. I can replace the pthread stuff with GCD.

@tgymnich tgymnich force-pushed the move-lock-to-file-system branch 4 times, most recently from 7cb4158 to eec4846 Compare September 20, 2020 23:35
@tgymnich tgymnich marked this pull request as draft September 20, 2020 23:35
@tgymnich
Copy link
Contributor Author

tgymnich commented Sep 20, 2020

The tests I've added for FileLock don't seem to run under Windows even before my changes. I keep getting TSCBasic.FileSystemError.unknownOSError(32).
Update: Fixed by changingdwShareMode in CreateFileW call.

@tgymnich
Copy link
Contributor Author

tgymnich commented Sep 21, 2020

The property wrapper @AutoClosing still feels a bit hacky. I am open for suggestions on wrapping a file descriptor and closing it on deinit. This is needed for the thread local storage to properly close the file descriptor.

Alternatives considered:

  1. Don't use a property wrapper, use a "normal" wrapper instead e.g. FileDescriptor
  2. Don't make FileLock thread safe. Just add a comment, that it's not thread safe.
  3. Use locks.

@tgymnich tgymnich force-pushed the move-lock-to-file-system branch 3 times, most recently from f2eddd7 to daa6525 Compare September 21, 2020 01:30
@tgymnich tgymnich marked this pull request as ready for review September 21, 2020 13:20
@neonichu
Copy link
Contributor

@swift-ci please test

@neonichu
Copy link
Contributor

@compnerd Do we have any separate Windows CI for TSC right now?

@tgymnich tgymnich force-pushed the move-lock-to-file-system branch from daa6525 to 0318587 Compare September 26, 2020 22:29
@neonichu
Copy link
Contributor

neonichu commented Oct 5, 2020

@swift-ci please smoke test

@shahmishal
Copy link
Member

The Swift project moved the default branch to main and deleted master branch, so GitHub automatically closed the PR. Please re-create pull request with main branch.

More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412

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.

4 participants