Cannot create multiple instances of AsyncService

[jelmervdl]

While working on translateLocally, I noticed that you cannot instantiate more than one instances of AsyncService at a time.

E.g. this doesn't work:

std::unique_ptr<AsyncService> service = make_unique<AsyncService>(config);
service = make_unique<AsyncService>(config);  // make_unique() never returns because constructor blocks

But this does:

std::unique_ptr<AsyncService> service = make_unique<AsyncService>(config);
service.reset(); // destructs the only AsyncService
service = make_unique<AsyncService>(config);  // make_unique() never returns because constructor blocks

I think it is related to Logger which is there to clean up spdlog's logs. The call to createLoggers(); there blocks the second time if I haven't released the previous AsyncService yet.

Anyway, in TranslateLocally I can work around this issue, it's not blocking development for me. But it is a bit unexpected.

[jelmervdl]

One way around it would be to do something like this:

class LoggerLock {
public:
	LoggerLock() {
		if (auto logger = shared_.lock()) {
			logger_ = logger;
		} else {
			logger_ = make_shared<Logger>();
			shared_ = logger_;
		}
	}
private:
	shared_ptr<Logger> logger_;
	static weak_ptr<Logger> shared_;
};

All BlockingService and AsyncService can then just make LoggerLock (LockerRef? Don't know what would be a good name) a member variable. When all services go out of scope, the shared_ptr will also go since the static weak_ptr won't count towards the reference count.

Edit: above code is not thread-safe. If you're creating AsyncService in multiple threads the LoggerLock constructor will have a race condition, and you'd need a static mutex for the else case to guarantee only one Logger instance is created.

Edit 2: maybe a better way would be to have Logger() have a private constructor and a static shared_ptr<Logger> Logger::sharedInstance() method that internally has the weak_ptr and mutex as static variables. That would also guarantee nobody accidentally calls the Logger constructor by hand.

[jerinphilip]

Linking #226 as something related.

Marian's logging is created by and referred to by string-names. I have attached these to the lifetime of AsyncService. So if you try to create two services at the same time there might be locks/blocks. It should still clean up after itself correctly though, right? Given service.reset() is destructing correctly?

How I'd want translateLocally to work is use a single AsyncService and swap models given the API now supports multiple models. There are some inefficiencies in the current approach in swapping models tracked in #257 which might make it less-favourable for now.

[jelmervdl]

It should still clean up after itself correctly though, right? Given service.reset() is destructing correctly?

I assume so. I haven't tried to construct a second instance in a second thread, then releasing the first instance in another thread. I currently just make sure there is only ever one instance, and releasing that one before I create a new one.

How I'd want translateLocally to work is use a single AsyncService and swap models given the API now supports multiple models.

I tried this, but noticed that the number of workers parameter is an argument to AsyncService. Since you can change that in TranslateLocally, I recreate the service to pass in the new number.

[XapaJIaMnu]

@jerinphilip can models be swapped if they have different vocabulary sizes? If they have completely different configurations? We support users providing their own models and people try to use this feature. Recreating the service is by far the simpler.

[jelmervdl]

Note for self/others: easier solution would be std::enable_shared_from_this(). The "best" example from that page seems appropriate here.