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

Created a better thread safety test and fixed a couple thread safety issues #395

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ivakegg
Copy link
Contributor

@ivakegg ivakegg commented May 7, 2023

No description provided.

@ivakegg ivakegg force-pushed the bugfix/vfsloader branch from 5dc421d to 46dbafb Compare May 7, 2023 11:24
* Updated the class loader tests to show the errors
* Updated ZipFileSystem and TarFileSystem with threadlocal files
@ivakegg ivakegg force-pushed the bugfix/vfsloader branch from 46dbafb to db94749 Compare May 7, 2023 11:26
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

-1 failed build and:
Quick note but the PR would need a detailed review from more folks: I am really skeptical here, VFS was not designed for thread safety; hacking some code here and there is more likely to make the code base much harder to understand and maintain. In the past, we've just told users they have to handle thread safety in their applications. Slapping "synchronized" here and there is likely to create side effects like dead-locks in existing applications. While we've that've made some changes in the past to help with thread safety (see the mailing list, jira, and PRs), doing that consistently is delicate and I am concerned about unintended side effects.

ivakegg added 2 commits May 10, 2023 14:33
* Updated test suite mechanism to allow exclusing test classes
* Skip threaded tests for http4 and ftps mechanisms
@ivakegg
Copy link
Contributor Author

ivakegg commented May 10, 2023

Well, given how many of the test cases are showing non-thread safety (as has been stated previously), I am going to change this to only test for thread safety where it is needed wrt the VFSClassLoader (e.g. jar/zip)

@ivakegg
Copy link
Contributor Author

ivakegg commented May 18, 2023

-1 failed build and: Quick note but the PR would need a detailed review from more folks: I am really skeptical here, VFS was not designed for thread safety; hacking some code here and there is more likely to make the code base much harder to understand and maintain. In the past, we've just told users they have to handle thread safety in their applications. Slapping "synchronized" here and there is likely to create side effects like dead-locks in existing applications. While we've that've made some changes in the past to help with thread safety (see the mailing list, jira, and PRs), doing that consistently is delicate and I am concerned about unintended side effects.

The only build failure is on mac OS 8 and the error does not appear to have anything to do with my changes. I understand the not designed for thread safety issues, however if you are going to supply a class loader implementation, then the underlying protocols being used will have to be thread safe.

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.

2 participants