A very trivial patch. Removes delegate() and takes a Codec in FilterCodec ctor. The changes to the 3 classes that extend it are simple.

if possible, I'd like to commit it to 4.0 too, just so that whoever extends FilterCodec in 4.0 doesn't need to change his code when he upgrades to 4.1. And the change is really trivial.

This is also similar to what i suggested on LUCENE-4391, see Adrien's comment.

If we do this, we should probably add a NOTE/WARNING somewhere that tells you
you should not call Codec.forName in your no-arg constructor.

e.g. dont call super("mycodec", Codec.forName(xxxx))

If we do this, we should probably add a NOTE/WARNING somewhere that tells you you should not call Codec.forName in your no-arg constructor.

+1

Maybe we should also test loader against null in Codec.forName to throw a better exception than a NPE whenever initialization issues happen (or even better fix this issue, but I am not sure it is possible...).

I can add to Codec.forName() something like <b>NOTE:</b> do not call this method from a Codec's ctor !. What do you think?

This is the same chicken-and-egg problem the ICU4J project had with one Japanese Locale. The problem here is, that Codec.forName() cannot be used before SPI loading is finished. The problem here is that SPI loads all codec names and creates instances from them. But at the time of creating those instances, the lookup does not yet work.

I agree we should add a null check in SPILoader's lookup method (not Codec.forName) to throw IllegalStateException("Cannot requests codec until service provider interface has finished loading them").

From the documentation perspective, the problem is not really FIlterCodec's problem. Its more general really.

And separately, i.e. separate issue, not for 4.0, I think we should make Lucene40Codec completely final. With FilterCodec, whoever wants to extend Lucene40Codec should just extend FilterCodec, like AppendingCodec, and use PerFieldPF. But this needs a separate discussion. I'll open a separate issue.

I would prefer something like this patch over the abstract method we currently have, especially if we have some way to throw an exception in the
SPI loader if you do a chicken-egg.

Can we make a separate patch to throw an exception if you do this? I definitely would support this null check for 4.0, because currently
you will get a confusing exception.

I agree we should add a null check in SPILoader's lookup method (not Codec.forName) to throw IllegalStateException("Cannot requests codec until service provider interface has finished loading them").

This won't help. The NPE is thrown from Codec.forName because loader is null ! It confused me, since I thought static members should be initialized before any method is accessed, but perhaps this doesn't hold for static methods?

From the documentation perspective, the problem is not really FIlterCodec's problem. Its more general really.

Yes, that's why I suggested to add this comment to Codec.forName(). Oh, unless you mean that we should add that comment not as part of this issue?

Maybe we should also test loader against null in Codec.forName to throw a better exception than a NPE whenever initialization issues happen (or even better fix this issue, but I am not sure it is possible...).

loader should be always != null. The problem is that NamedSPILoader.lookup() may NPE/fail. I can at least add a test that verifies what happens when you do call Codec.forName() inside a Codec ctor.

Ah you are right, it might be == null. Beacuse at the time of init (until NamedSPILoader's ctor has finished), the assignment is not yet done. So we must null-check in Codec class itsself to throw Exception.

Inside NamedSPILoader loader all is fine, because the List is initialized to be empty initially and later replaced by a unmodifiable reloaded one.

Isn't there a way to ensure that NamedSPILoader would finish before forName() is accessed? If not, we need to add such check to every forName we have. Here's a proposal message:

    if (loader == null) {
      throw new IllegalStateException("you are probably calling this method from inside a Codec's ctor); you shouldn't!");
    }

Please add this also to listCodecs maybe

Isn't there a way to ensure that NamedSPILoader would finish before forName() is accessed?

Unfortunately not, this is a chicken-and-egg problem: If you access Codec.class in any way (e.g. already at the time when you subclass it!), it is classloaded. After classloaded, it is initialized, so its static <clinit> method is called. This method runs new NamedSPILoader(Codec.class) [the first line in the class source code], and this one loads all codecs which are listed in META-INF/services. As e.g. AppendingCodec is listed there, it is loaded by NamedSPILoader and its ctor is called. As we are still in the stacktrace inside NamedSPILoader.<init>, this call did not yet return to caller (Codec.<clinit>), so the assignment of the static final loader field is not yet finished. Unfortunately, AppendingCodec.<init> calls Codec.forName() and hits the NPE, because the field is not yet initialized. You have to say: You cannot call any method of Codec at that time!

Patch adding the Exceptions to Codec and PostingsFormat.

The Analysis SPI factory loading mechanism does not have the chicken-egg problem:

• It returns new instances and has no forName() method.
• Static class ctors from Subclasses are not executed while SPI loading, ass SPIClassIterator does not initialize classes while loading. The first time a factory is touched is when a new instance is created and that happens not while SPI is initializing

Please check the messages, I want to commit this ASAP, because Robert wants to spin a new RC.

thanks for adding these checks: I think this is much more clear than NPE!

Committed trunk revision: 1391140
Committed 4.x branch revision: 1391143
Committed 4.0 branch revision: 1391144

Thanks Uwe for adding the messages and committing.

[branch_4x commit] Uwe Schindler
http://svn.apache.org/viewvc?view=revision&revision=1391143

Merged revision(s) 1391140 from lucene/dev/trunk:
LUCENE-4440: Refactor FilterCodec to get the delegate passed into ctor. Fix Codec's and PostingsFormat's forName lookup methods to throw IllegalStateException if you call those methods from the constructor of e.g. FilterCodec.