Hi Sergey,
> 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)".
That's not true, because if a plugin throws e.g. "RuntimeException",
AudioSystem would have always passed it on. So continuing to pass it
on is OK.
I agree that the code would be cleaner without that catch clause.
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.
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
|
