Make primary instance initialized lazily #407
Conversation
|
|
|
It's unclear to me when this would be necessary, unless you are running multiple class-loaders, static field initialization is already thread-safe (AFAIK). and if that was a case I'm not sure your AtomicBoolean would be in any better? can you point me to some documentation on when this would be needed? |
The current approach is thread-safe but it is not lazy. Thus even if a user wants to use instance-based approach (i.e. without using primary instance) the primary instance will still be In order to solve this, I've moved the primary instance into a separate class so that it gets lazily initialized on actual first request to it. As for the In other wordsI didn't mean to say that current implementation is not thread-safe (there may be some thread-unsafe spots in I did mean to say that the reason for using a separate holder-class was to provide thread-safety with lazy initialization. Real-life exampleA server which uses plugin system to load extensions at runtime: there is an API-plugin (let's call it RestApi Plugin for simplicity) which shades Unirest in so that dependent plugins can use it without relocating it to their own packages. Yet each plugin is required to spawn their own As a maintainer of RestApi Plugin I take the responsibility of providing the shaded |
|
Ah! Yes, thank you for the explanation. this is a good idea. A couple of things:
|
I've pushed c586a00 to resolve it;
As of this, I am not that sure. There are tools like JCStress (Maven Central) but these seem to fit more for low-level scenarios as it's hard to attach any monitors to the internal singleton to check its state. As I see, the only possible spot in which I cannot be sure about correct ordering is scenario:
Thus there are twe following possible invariants (considering the fact that all threads are guaranteed to observe the
IMO, first two scenarios in which (for some reason) shut down was called before initialization do not lead to weird behaviour (or it is the caller's fault that he first managed to issues shut down and then use the instance from which he is not guarded with current implementation too). The last scenario is just correct and does not seem to produce any logical problems.
|
|
The change is failing the tests. This is because now, once the instance is shut down it's shut down forever. In the old code, while it's true that the UnirestInstance was created. It's configuration was lazy and the core Apache classes (and their various threadpools) were lazy. Calling "shutdown" on an instance shuts everything off and then puts the configuration back into a dormant state which then can be re-initilized. The tests take advantage of this and shut down the instance after each test, so now only the first shutdown in all the suites works. I'd like to say this is just a weird thing in the tests but I've spoken with multiple folk who have taken advantage of this in real life. If we change the instance holder to allow for a null state I think we can avoid that. This would require some synchronized methods which we might want to avoid in a heavy traffic situation so theres a little dance here. I'm not saying this is the best answer, I'm open to improvements, but this does what we want I think AND passes all the tests. We would then have Unirest delegate to the methods rather than looking at the instance itself. /**
* Internal holder class which allows lazy initialization of primary {@link UnirestInstance}.
*/
private static final class Primary {
/**
* Instance of the Unirest runtime lazily initialized on startup.
*/
private static UnirestInstance instance;
private static UnirestInstance getInstance() {
if(!exists()) {
init();
}
return instance;
}
public static boolean exists() {
return instance != null;
}
private static synchronized void init() {
if(!exists()) {
instance = new UnirestInstance(new Config());
}
}
public static synchronized void shutDown(boolean clearOptions) {
if(exists()){
instance.shutDown(clearOptions);
instance = null;
}
}
public static boolean isRunning() {
return exists() && instance.isRunning();
}
private Primary() {
throw new AssertionError("Primary should never be constructed");
}
} |
Have you considered using https://byteman.jboss.org/ ? |
|
One nice thing about using the null pattern would be it's easily tested. because we can call the exists() method. |
This one may requires some changes (I will ltry to imlpement these) as because there are no means of synchronization between method calls, one thread can actually observe I will try to implement a safer variant with support for switching between initialized and shut down states so that the tests pass. Considering the problems which appear now while doing this, may I suggest moving the This would allow (as for the example below) not to rely on primary instance existence (if you don't use This is OK from point of semver as the major version gets changes allowing backwards-incompatible changes and still it allows more implementation freedom from point of regular |
Wouldn't the synchronized on the method stop that? Unless you had threads fighting to shut down and re-init the instance at the exact moment in which case 🤷 . |
It may but JMM is quite tricky thus I think abot a bit more specific approach.
Yes! That's the exact problem with trying to proof the correctness of the approach) |
|
Just to be clear about one part of all this:
this is not true, a configuration exists, but it's not initialized, It's a few classes in memory thats true, but the real engine, the thing that we need to be aware of on application shutdown, are the Apache clients, in an uninitialized configuration those are two empty optionals. So there are no threadpools or anything else going on. Shutting down only shuts them down if they exist, otherwise it's a noop. |
Hm, that's interesting. My bad that I haven't considered this initially. Funnily, looking at the implementation details this means that the problems possibly appearing in singleton design above (the one with separate synchronized methods) also appear on unirest-java/unirest/src/main/java/kong/unirest/Config.java Lines 766 to 771 in 785d6cc first calling buildAsyncClient writing to asyncClient() and then invoking getFinalAsyncClient() which relies on this field which yet may have been updated to Optional.EMPTY concurrently.
From this perspective this PR may really provide more problems than profit (as you are right that current implementation is already lazy enough). In this case, is there any chance that the suggestion from #407 (comment) makes its way into |
|
4.0 Unirest doesn't use Apache, and there are no background monitors or anything. At least as of right now .... so it's even less of a concern there at the moment. Because there isn't anything to "start" or "shut down". |
|
closing as stale |
Description
This makes the primary instance initialization lazy so that it gets thread-safely initialized on first reqeuest.