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
