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

Improve performance for operations that modify framework state #54

Merged
merged 2 commits into from
May 16, 2022

Conversation

tjwatson
Copy link
Contributor

Many places in the internal management of framework state use File.createTempFile to stage updates before moving them to their final location on disk. The issue is File.createTempFile uses SecureRandom to generate unique file names. This is a bit overkill for our usages and can cause slowdown on some environments when used extensively.

This change implements a simple version of createTempFile that can be used for only internal purposes for the framework.

File.createTempFile uses SecureRandom under the covers to generate
unique file names.  This may be important for when storing files in the
global temporary file space to avoid something guessing the file and
writing to it.

The usage in the framework for createTempFile is always used for staging
file content before it gets (reliably) moved to its final destination.
This always happens as an implementation detail of storing some state in
the internal file storage of the framework.  Risk is low that some
malicious code would be able to predict the temporary file and somehow
interfere with the behavior of the framework.  Once some malicious code
has had access to the internal storage of the framework they will be
able to write to other files that have very predictable names to cause
issues.
If we really have this many temporary files
there is something wrong that needs to be addressed.
Fail with IOException if we detect a 100000 attempts
to find a non-existing file
@tjwatson tjwatson force-pushed the creatTempFileNoSecureRandom branch from 03d3eae to 56fcf01 Compare May 16, 2022 20:01
@tjwatson tjwatson merged commit fa10a40 into eclipse-equinox:master May 16, 2022
@laeubi
Copy link
Member

laeubi commented May 17, 2022

@tjwatson just wondering: Does it really matters how the file is named? Maybe instead of using a prefix, one could simply use an UUID.randomUUID() as a start, then, if the file exits simply add a (local) incrementing number, this should not require a global counter and in almost all cases give a unique filename at the first try.

@tjwatson
Copy link
Contributor Author

Maybe instead of using a prefix, one could simply use an UUID.randomUUID() as a start, then

UUID.randomUUID() uses SecureRandom also, so would be no different than using File.createTempFile.

@tjwatson
Copy link
Contributor Author

One bit of detail I left out so far. The motivation for this change is for systems where there is contention for the system resource that produces random numbers from the OS (e.g. /dev/urandom on Linux). If we can avoid this contention it helps for system that may have multiple processes making system calls to get random numbers. For example, multiple containers running with runc (or docker).

@laeubi
Copy link
Member

laeubi commented May 17, 2022

Never have had though that this is an issue at all, is the OSGi framework creating that many files?

@tjwatson
Copy link
Contributor Author

Never have had though that this is an issue at all, is the OSGi framework creating that many files?

The answer is, it depends. In one scenario I was investigating recently it was a testcase running many different instances of the framework and each one installing lots of bundles. The staging happens for each bundle stored in the framework. This ultimately lead to a bottleneck and some timeouts in our tests.

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.

3 participants