<Sound Dev> [9] Review Request: 8135100 Behavior of null arguments not specified in javax.sound.sampled.spi
Florian Bomers
javasound-dev at bome.com
Fri Oct 30 21:28:34 UTC 2015
+1
thanks!
On 30.10.2015 14:26, Alex Menkov wrote:
> The fix looks ok for me.
>
> regarda
> Alex
>
> On 28.10.2015 21:14, Sergey Bylokhov wrote:
>> Hi, Florian.
>> Thanks for review!
>> On 26.10.15 18:41, Florian Bomers wrote:
>>> But I don't agree with "bug to bug compatiblity" and "fail fast":
>>> IMHO,
>>> backwards compatibility is much more important. The only exception
>>> is if
>>> you cannot align an important bug fix with 100% backwards
>>> compatibility.
>>> But removing this "catch NPE clause" is not an important bug fix.
>>
>> I am sure that I understand your point of view, I just want to
>> highlight, that because of this silent behavior we preserve the bugs in
>> the plugin code, which can cause some unpredictable issues.
>>
>> Well it seems there are no any other opinions, I have updated the fix,
>> The catch of npe is reverted and the comment why it was added, was
>> updated:
>> http://cr.openjdk.java.net/~serb/8135100/webrev.04
>>
>>>
>>> Of course, this is a rather unimportant case. Chances are that there
>>> does not exist a broken MixerProvider out in the wild. But the
>>> discussion is worthwhile for setting the priorities for other bug
>>> fixes
>>> with similar trade-offs.
>>>
>>> Arguing that users can and should just remove the failing plugin
>>> ignores
>>> many other deployment possibilities of Java code. For example, code
>>> can
>>> be deployed centrally in binary form, and users don't have a choice of
>>> adding, updating, or removing components on their own. Another example
>>> is where users are stuck with binary code which they need but cannot
>>> change anymore. For the user, knowing that a plugin is broken, is
>>> not of
>>> much help when she cannot fix the plugin... Also, of course, it is
>>> possible that a MixerProvider only throws NPE's under certain
>>> conditions
>>> and works fine under other conditions. So removing that MixerProvider
>>> would remove its legitimate function.
>>>
>>> In my opinion, updating to a new JRE should never break existing
>>> applications -- if at all possible. Of course, this is my view. I
>>> don't
>>> know what Oracle's current policy is on this. And I don't know what
>>> other incompatibilities JDK 9 will bring. But it does look to me that
>>> existing service providers will still be supported.
>>>
>>> Thanks,
>>> Florian
>>>
>>>
>>>>>
>>>>> Removing the NPE catch clause, however, will still cause a backwards
>>>>> incompatibility, because if a poorly programmed MixerProvider gets
>>>>> installed which throws NPE for whatever reason (might also happen
>>>>> when
>>>>> "info" is non-null), now AudioSystem.getMixer() will throw NPE,
>>>>> where
>>>>> it previously worked.
>>>>
>>>> But if we discuss in this way we can get an assumption that any
>>>> methods of plugins can throw any type of exceptions and we should
>>>> wrap
>>>> them in "catch (Throwable)". I agree that we should follow the
>>>> backwards compatibility as much as possible, but this case is related
>>>> to "bug to bug" compatibility. For sure if we fix a bug in jdk and if
>>>> some code relies on this behavior he will get an issue. But i think
>>>> strategy of "fail-fast" is better, than silently ignoring the problem
>>>> since a workaround should be simple as mixer removing, which is not
>>>> used anyway.
>>>>
>>>> Note that in case of jdk9 an additional check of application's sound
>>>> code will be needed, because the order of serviceloaders will be
>>>> different, modules which contain providers come to play, etc. So I
>>>> guess this is a good time to cleanup of our code from some
>>>> workarounds
>>>> which were added in jdk 1.1.*.
>>>>
>>>>>
>>>>> I agree that it's harder for debugging mixer providers if NPE is
>>>>> ignored. Other than that, I don't see any problem with keeping
>>>>> the NPE
>>>>> catch for backwards compatibility's sake. Even if just
>>>>> theoretical...
>>>>> But you never now, companies might be using poorly programmed
>>>>> in-house
>>>>> software or the like.
>>>>
>>>>> Hi, Florian.
>>>>> Thanks for review! see my comments inline.
>>>>>
>>>>>> This is true for the included MixerProvders, but the requirement
>>>>>> that
>>>>>> null will return the default Mixer is just made official. We
>>>>>> should,
>>>>>> however, remain backwards compatible with 3rd party
>>>>>> MixerProviders by
>>>>>> keeping that second loop. The old style is that a MixerProvider
>>>>>> returns its default Mixer as first element. Also, for backwards
>>>>>> compatibility, I'd also keep the catch clause for NPE in both
>>>>>> loops.
>>>>>
>>>>> This is tricky place, I have some thoughts about this, which I would
>>>>> like to discuss.
>>>>>
>>>>> I studied the history to find an answer why the catch of NPE and the
>>>>> second loop were added.
>>>>>
>>>>> - The catch of the null was added in the 1999 because of "added a
>>>>> catch
>>>>> for an NPE -- Netscape tends to throw this if some strings
>>>>> haven't been
>>>>> set in our device provider info objects".
>>>>> It is actually a workaround. Because of this patch we did not catch
>>>>> some
>>>>> bugs in our MixerProviders. For example PortMixerProvider,
>>>>> SimpleInputDeviceProvider(old mixer) were thrown NPE, when they
>>>>> tried to
>>>>> throw IllegalArgumentException. In the same moment the
>>>>> DirectAudioDeviceProvider, HeadspaceMixerProvider(old mixer),
>>>>> SoftMixingMixerProvider are ready for null. I also checked the
>>>>> TMixerProvider from tritonus it also ready for null.
>>>>>
>>>>>
>>>>> - The second loop was added in "JDK-4834461: Linux: Applet hang when
>>>>> you
>>>>> load it during sound card is in use". This is also a workaround
>>>>> for a
>>>>> situation when we try to get a default mixer in the first loop
>>>>> but it
>>>>> was not available for some reason, in this case we will return first
>>>>> mixer from the first mixer provider. But the second loop will be run
>>>>> only if the first loop will not find the default mixer in some other
>>>>> providers.
>>>>>
>>>>> So imagine this situation when some old 3rd party MixerProvider is
>>>>> used:
>>>>> - The user sets some provider, which throw the NPE on null in
>>>>> getMixer();
>>>>> - The first loop call this provider and skip it in catch block
>>>>> - The next bundled provider will be used instead of user's mixer,
>>>>> the
>>>>> second loop is not executed.
>>>>> - If we use this approach the user will not be able to check that
>>>>> wrong
>>>>> provider is in use, and in case of NPE the temporary workaround
>>>>> will be
>>>>> - removing of custom mixer provider which are not used anyway.
>>>>>
>>>>> Note that in jdk9 the "META-INF/services" will not be used, so there
>>>>> will be no option to remove bundled providers via configs, and we
>>>>> will
>>>>> always iterate over the bundled providers in the first loop.
>>>>>
>>>>> For the case of some other vendors of jdk, after the moment of
>>>>> specification clarification all default providers should not
>>>>> contradict
>>>>> the specification, and should not throw NPE in getMixer(), but
>>>>> return
>>>>> default mixer.
>>>>>
>>>>> Does it make sense?
>>>>>
>>>>>>
>>>>>>
>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-4834461
>>>>>>
>>>>>>
>>>>>> On 08.10.15 11:46, Florian Bomers wrote:
>>>>>>>> For me the most logical is to return default playback mixer :)
>>>>>>>
>>>>>>> yes, at the time it was most important to provide an easy way to
>>>>>>> get a
>>>>>>> playback device.
>>>>>>>
>>>>>>> Can you send a new webrev?
>>>>>>> Thanks,
>>>>>>> Florian
>>>>>>>
>>>>>>>
>>>>>>> On 25.09.2015 20:33, alexey menkov wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.09.2015 20:42, Sergey Bylokhov wrote:
>>>>>>>>> On 25.09.15 20:32, alexey menkov wrote:
>>>>>>>>>> Ok, lets only clarify MixerProvider.getMixer(null) behavior.
>>>>>>>>>> Actually AusioSystem.getMixer(null) looks unclear because
>>>>>>>>>> unclear
>>>>>>>>>> what
>>>>>>>>>> is the "default mixer" in the case (we may have different
>>>>>>>>>> "default
>>>>>>>>>> playback mixer", "default recording mixer", "default port
>>>>>>>>>> mixer").
>>>>>>>>>
>>>>>>>>> Right, if to consider that the sequence of providers isn't
>>>>>>>>> specified,
>>>>>>>>> then it is unclear what this method should actually return. I do
>>>>>>>>> not
>>>>>>>>> understand the purpose to return some random mixer. I think that
>>>>>>>>> intention was to return "default playback mixer"?
>>>>>>>>
>>>>>>>> I don't know. I suppose this is ancient method (and most likely
>>>>>>>> nobody
>>>>>>>> use it) and the implementation was changed this way to get
>>>>>>>> DirectAudioDevice as default (it supports both playback and
>>>>>>>> recording).
>>>>>>>> For me the most logical is to return default playback mixer :)
>>>>>>>>
>>>>>>>> --alex
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> regards
>>>>>>>>>> Alex
>>>>>>>>>>
>>>>>>>>>> On 25.09.2015 18:41, Sergey Bylokhov wrote:
>>>>>>>>>>> Hi, Alexey.
>>>>>>>>>>> Thanks for review! see my comments inline.
>>>>>>>>>>>
>>>>>>>>>>> On 25.09.15 18:23, alexey menkov wrote:
>>>>>>>>>>>> Hi Sergey,
>>>>>>>>>>>>
>>>>>>>>>>>> Overall looks good, but I don't like change in
>>>>>>>>>>>> MixerProvider.getMixer
>>>>>>>>>>>> and PortMixerProvider.getMixer.
>>>>>>>>>>>>
>>>>>>>>>>>> MixerProvider.getMixer(null) is used by
>>>>>>>>>>>> AudioSystem.getMixer(null) (to
>>>>>>>>>>>> get the default mixer), but provide PortMixer as the default
>>>>>>>>>>>> does
>>>>>>>>>>>> not
>>>>>>>>>>>> look good, I'd expect AudioSystem.getMixer(null) returns some
>>>>>>>>>>>> playback-able device. (Note also that for Ports
>>>>>>>>>>>> "SourceDataLine"
>>>>>>>>>>>> means
>>>>>>>>>>>> controls for recording device)
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Also comment for MixerProvider.getMixer(Mixer.Info):
>>>>>>>>>>>> > * @param info an info object that describes the desired
>>>>>>>>>>>> mixer,
>>>>>>>>>>>> > * or {@code null} for the system default mixer
>>>>>>>>>>>>
>>>>>>>>>>>> is unclear and is not consistent with the description above:
>>>>>>>>>>>> > * The full set of the mixer info objects that
>>>>>>>>>>>> represent the
>>>>>>>>>>>> mixers
>>>>>>>>>>>> > * supported by this {@code MixerProvider} may be obtained
>>>>>>>>>>>> through
>>>>>>>>>>>> the
>>>>>>>>>>>> > * {@code getMixerInfo} method. Use the {@code
>>>>>>>>>>>> isMixerSupported}
>>>>>>>>>>>> method to
>>>>>>>>>>>> > * test whether this {@code MixerProvider} supports a
>>>>>>>>>>>> particular
>>>>>>>>>>>> mixer.
>>>>>>>>>>>> It looks like MixerProvider.getMixerInfo should add "null" to
>>>>>>>>>>>> the
>>>>>>>>>>>> supported mixers and MixerProvider.isMixerSupported(null)
>>>>>>>>>>>> should
>>>>>>>>>>>> return
>>>>>>>>>>>> "true".
>>>>>>>>>>>
>>>>>>>>>>> In this case the null means that some default mixer will be
>>>>>>>>>>> returned. I
>>>>>>>>>>> am not sure that isMixerSupported(null) should return true,
>>>>>>>>>>> instead I
>>>>>>>>>>> can clarify the specification of getMixer(null), and mention
>>>>>>>>>>> that if
>>>>>>>>>>> null is provided then this mixer will try to return some
>>>>>>>>>>> default(
>>>>>>>>>>> supported) mixer if possible, otherwise
>>>>>>>>>>> IllegalArgumentException
>>>>>>>>>>> will be
>>>>>>>>>>> thrown.
>>>>>>>>>>>
>>>>>>>>>>> For the case of PortMixerProvider and "null" I can throw a
>>>>>>>>>>> IllegalArgumentException which will mean that this provider do
>>>>>>>>>>> not
>>>>>>>>>>> have
>>>>>>>>>>> "default" mixer.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> For now I don't have a proposal how to fix this.
>>>>>>>>>>>> Maybe it would be better to fix behavior of
>>>>>>>>>>>> AudioSystem.getMixer(null) -
>>>>>>>>>>>> now it ignores "sound.properties" file (see AudioSystem
>>>>>>>>>>>> spec for
>>>>>>>>>>>> explanations).
>>>>>>>>>>>>
>>>>>>>>>>>> regards
>>>>>>>>>>>> Alex
>>>>>>>>>>>>
>>>>>>>>>>>> On 14.09.2015 16:29, Sergey Bylokhov wrote:
>>>>>>>>>>>>> Hello Audio Guru.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please review the fix for jdk9. This issue is a subtask of:
>>>>>>>>>>>>> 4912693: Behavior of null arguments not specified in Java
>>>>>>>>>>>>> Sound
>>>>>>>>>>>>>
>>>>>>>>>>>>> In this patch I cover the whole javax.sound.sampled.spi
>>>>>>>>>>>>> package.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The small description of the fix:
>>>>>>>>>>>>> - I have checked all methods in the spi package and all
>>>>>>>>>>>>> related
>>>>>>>>>>>>> methods
>>>>>>>>>>>>> in AudioSystem class.
>>>>>>>>>>>>> - I have moved related tests to the folder corresponding the
>>>>>>>>>>>>> package
>>>>>>>>>>>>> and
>>>>>>>>>>>>> class name.
>>>>>>>>>>>>> - I have written a tests for every method and class which I
>>>>>>>>>>>>> changed.
>>>>>>>>>>>>> Note that these classes related to the different service
>>>>>>>>>>>>> providers,
>>>>>>>>>>>>> so I
>>>>>>>>>>>>> have covered all installed implementations of each provider.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Long description.
>>>>>>>>>>>>> I splits the fix to 3 use cases:
>>>>>>>>>>>>> - If the method always throw a NPE, then I simply update a
>>>>>>>>>>>>> javadoc and
>>>>>>>>>>>>> write a small test.
>>>>>>>>>>>>> - If the method most of the time throw a NPE then I update a
>>>>>>>>>>>>> javadoc
>>>>>>>>>>>>> and
>>>>>>>>>>>>> change the method to always throw a NPE. Also I write a test
>>>>>>>>>>>>> which
>>>>>>>>>>>>> tries
>>>>>>>>>>>>> to emulate both cases when NPE was thrown and when not. For
>>>>>>>>>>>>> example
>>>>>>>>>>>>> AudioFileWriter.isFileTypeSupported(Type) always throws a
>>>>>>>>>>>>> NPE
>>>>>>>>>>>>> if at
>>>>>>>>>>>>> least one type is supported, but if the array is empty then
>>>>>>>>>>>>> false is
>>>>>>>>>>>>> returned.
>>>>>>>>>>>>> - If the method have a few parameters and throw a NPE for
>>>>>>>>>>>>> some
>>>>>>>>>>>>> set of
>>>>>>>>>>>>> them. For example AudioFloatFormatConverter.
>>>>>>>>>>>>> isConversionSupported(Encoding,AudioFormat), the appropriate
>>>>>>>>>>>>> test
>>>>>>>>>>>>> tries
>>>>>>>>>>>>> to cover these cases.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It turned out that all methods throw a NPE except of one:
>>>>>>>>>>>>> AudioSystem.getMixer()(MixerProvider.getMixer()), but it was
>>>>>>>>>>>>> found
>>>>>>>>>>>>> that
>>>>>>>>>>>>> the specification of MixerProvider.getMixer has no
>>>>>>>>>>>>> information
>>>>>>>>>>>>> about
>>>>>>>>>>>>> the
>>>>>>>>>>>>> null, so I copied it from the AudioSystem.getMixer().
>>>>>>>>>>>>> Also one
>>>>>>>>>>>>> implementation of MixerProvider -
>>>>>>>>>>>>> PortMixerProvider.getMixer()
>>>>>>>>>>>>> throws
>>>>>>>>>>>>> NPE, so updated its implementation to the same as
>>>>>>>>>>>>> DirectAudioDeviceProvider.getMixer();
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have done all related regression/jck/sqe tests, and I
>>>>>>>>>>>>> found
>>>>>>>>>>>>> one
>>>>>>>>>>>>> issue
>>>>>>>>>>>>> in jck and regression tests. Both are related to JDK-4941629
>>>>>>>>>>>>> [1]
>>>>>>>>>>>>> (see
>>>>>>>>>>>>> comments in this CR). The jck test assumes that the method
>>>>>>>>>>>>> AudioSystem.write(ais, null, stream) should throw
>>>>>>>>>>>>> IllegalArgumentException. But according to specification it
>>>>>>>>>>>>> should
>>>>>>>>>>>>> throw
>>>>>>>>>>>>> IllegalArgumentException if the type is unsupported, but the
>>>>>>>>>>>>> related
>>>>>>>>>>>>> method AudioSystem.isFileTypeSupported(Type) will always
>>>>>>>>>>>>> throw
>>>>>>>>>>>>> a NPE
>>>>>>>>>>>>> for null. I prefer to file a bug against jck for this case.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-4941629
>>>>>>>>>>>>>
>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8135100
>>>>>>>>>>>>> The new test:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~serb/8135100/webrev.01
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Florian Bomers
>>>> Bome Software
>>>>
>>>> everything sounds.
>>>> http://www.bome.com
>>>> __________________________________________________________________
>>>> Bome Software GmbH & Co KG Gesellschafterin:
>>>> Dachauer Str.187 Bome Komplementär GmbH
>>>> 80637 München, Germany Geschäftsführung: Florian Bömers
>>>> Amtsgericht München HRA95502 Amtsgericht München HRB185574
>>>>
>>
>>
>
--
Florian Bomers
Bome Software
everything sounds.
http://www.bome.com
__________________________________________________________________
Bome Software GmbH & Co KG Gesellschafterin:
Dachauer Str.187 Bome Komplementär GmbH
80637 München, Germany Geschäftsführung: Florian Bömers
Amtsgericht München HRA95502 Amtsgericht München HRB185574
More information about the sound-dev
mailing list