Hi Brian,
Thanks for digging deeper and the recap.
I don't see any cases in which it is necessary or valuable to allow
\0 in Strings (key or value). The original bug report did not indicate
whether
it was discovered as a testing exercise or when diagnosing a bug in an
application.
The compatibility risk is unknown at the moment since no cases have been
noted that require \0 in strings.
Long term the code will be easier to maintain if it is less complex and
has fewer
variables that are platform or format specific and developer errors are
detected
as soon as possible.
Of the possible remedies below, #2 seems the most practical. But it will
further
hide the problem from developers since calling flush and sync is never
required
for correct operation:
///"explicit //flush//invocation is //not//required upon termination to
ensure that pending updates are made persistent"/
Flush and sync would be the only opportunity to throw an exception and
explain the cause.
It will be harder to track down since the true cause will be far removed
from
the time/location of the exception.
In the absence of advice to continue to support \0in Preference strings
on Windows
or Mac I'd continue with the recent direction to make \0 in key and
value strings throw IAE.
Roger
On 4/10/2015 6:26 PM, Brian Burkhalter wrote:
On Apr 4, 2015, at 12:53 PM, Alan Bateman <alan.bate...@oracle.com> wrote:
On 24/03/2015 19:20, Brian Burkhalter wrote:
Please review at your convenience.
Issue: https://bugs.openjdk.java.net/browse/JDK-8075156
Patch: http://cr.openjdk.java.net/~bpb/8075156/webrev.00/
This is a sequel to the resolved issue
https://bugs.openjdk.java.net/browse/JDK-8068373, (prefs) FileSystemPreferences
writes \0 to XML storage, causing loss of all preferences, wherein the code
point U+0000, the null control character, was made illegal to use as a key in
the generic Unix file system-based Preferences.
The issue at hand extends disallowing U+0000 as a key in the put() method on
Mac OS X and Windows, and also disallows this use to the remove() methods on
these platforms and in the generic Unix file system-based Preferences.
Use of U+0000 in the corresponding get() method has not been disallowed as this
method returns a default value. If it would be preferable that the behavior of
get() with respect to the key U+0000 were the same as for put() and remove()
then this patch may be updated to that effect.
Minimally then the putXXX methods should make it clear that they throw IAE for
this case. This would be a javadoc only change because the implementation does
this as a consequence of the original patch.
Agreed.
Actually I am not completely satisfied with the fix for
https://bugs.openjdk.java.net/browse/JDK-8068373 so I went back over all the
discussions and notes on the various ways to fix the problem trying to rethink
whether there might be a better solution. The problem is not really with the
Preferences APIs per se, but rather with the ability of the XML-based backing
store to handle all characters which might be present in a String, in this
particular case \u0000, but there are other possible problematic characters as
well.
Ideally, the fix would be to modify the XML backing store to handle all
possible characters. This does not however appear to be possible without
introducing an incompatibility when the XML backing store is shared between the
set of newer versions of Java which would support storing all characters and
the older versions which do not. There are ways to minimize the incompatibility
but not apparently eliminate it and the additional complexity might not merit
the effort.
In the RFC thread on 8068373, it was concluded that it would be better simply
to disallow \u0000 for XML-backed Preferences. This change however introduced
an incompatibility with non-XML backing stores which might or might not be able
to handle \u0000. So to address this incompatibility (and to address the
companion get/remove methods) the present RFR was introduced. The present
change has the potential however to break Preferences implementations which
have a backing store for which \u0000 *is* legal.
Note also that all Preferences implementations should be able to handle all
Strings if there is no interaction with the backing store, even in the case of
the XML backing store. Without a round trip via the XML backing store no error
would be encountered. This suggests an alternative fix of allowing the illegal
character to be used “live” but disallowed from being written to a backing
store which cannot handle it.
I raise these alternatives here because if any were preferable, then there is
no point in going further in the current direction as one changeset reversion
would already be needed.
In the original discussion then it was just a question as to whether get/getXXX
and remove should be consistent. If the get and remove methods will always
behave as if the key doesn't exist (and return the default value in the case of
get) then it shouldn't require a javadoc change.
In the case of get(), for implementations based on AbstractPreferences, any
exception thrown by getSpi() will in any case be caught and the default value
returned.
For remove() there might after all be no point in checking the key if the key
is disallowed from being stored in the first place, so the method call would be
a no-op and neither an implementation nor a javadoc change would be needed.
However I suspect it will require an implementation change as there may be
non-XML backing stores might that allow \0 in the key (hence get and remove
should actually do something).
As noted above, it might not be correct to extend the checked-in fix for
8068373 to the non-XML backing store cases as is being proposed in this RFR and
likewise not to modify remove() symmetrically.
Rethinking this entire topic l am now inclined to suggest retracting the prior
fix of 8068373 and instead take one of the following approaches:
1. Modify the XML backing store to be able to handle at least \0 if not other
characters problematic for XML. As noted above this might be complicated and
introduce inter-version incompatibilities.
2. Allow \0 to be in Strings at the API level but do not write these entries to
an XML backing store. This would allow in-session use of unsupported characters
but these would magically disappear thereafter which could cause confusion.
3. Change the put() specification to allow throwing an IAE if the backing store
cannot handle the supplied Strings. This however runs somewhat counter to the
Preferences class-level specification that "The user of this class needn't be
concerned with details of the backing store.”
On reexamining things I also noticed this paragraph in the AbstractPreferences
class-level specification:
"The remaining SPI methods putSpi(String,String), removeSpi(String) and
childSpi(String) have more complicated exception behavior. They are not specified to
throw BackingStoreException, as they can generally obey their contracts even if the
backing store is unavailable. This is true because they return no information and their
effects are not required to become permanent until a subsequent call to
Preferences.flush() or Preferences.sync(). Generally speaking, these SPI methods should
not throw exceptions. In some implementations, there may be circumstances under which
these calls cannot even enqueue the requested operation for later processing. Even under
these circumstances it is generally better to simply ignore the invocation and return,
rather than throwing an exception. Under these circumstances, however, all subsequent
invocations of flush() and sync should return false, as returning true would imply that
all previous operations had successfully been made permanent."
This suggests that solution #2 above might be the appropriate fix instead of
that of 8068373 plus the patch in the current RFR. There are errors in the
above specification paragraph however which should also be fixed, viz.,
flush{Spi}() and sync{Spi}() are void, not boolean-returning methods.
Thanks,
Brian
