<Sound Dev> [9] Review Request: 8135100 Behavior of null arguments not specified in javax.sound.sampled.spi

Wed Oct 28 18:14:42 UTC 2015

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
>>

-- 
Best regards, Sergey.