+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
