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

Alex Menkov alexey.menkov at oracle.com
Fri Oct 30 13:26:12 UTC 2015 

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

More information about the sound-dev mailing list