FWD » Core Development » Runtime Infrastructure

The remote OpenClient calls are managed in FWD via the RemoteLegacyJavaAppserverClient class, created via LegacyJavaAppserverClient.connectRemote; current implementation is pretty basic: it allows establishing a connection, but the same connection will not allow any pooling or otherwise limit the number of concurrent requests.

• the number of the concurrent, remote, calls using FWD OpenClient, from the 3rd-party application is limited only by that application (which may be tomcat/jetty web threads, or something else completely). This must be 'in sync' with the number of Dispatcher threads available in the FWD server, as all requests will end up on a Dispatcher threads. If we allow n+10 concurrent requests from the remote app, and FWD has only n Dispatcher threads, the FWD server can get blocked for other requests/management.
• each call requires a connect, execute call, disconnect. This is expensive in terms of obtaining the FWD connection.

• using the connect/call/disconnect model, it took ~24 seconds for all to complete
• obtaining the connection once, and the 5 threads sharing the same AppServerHelper instance, it took ~4.7 seconds.

Greg: the assumption is that the AppServerHelper instance and the connection to the FWD server allows asynchronous, concurrent, calls. I think this stands and is OK.

• connectRemote will use a cache of FWD server connections (using as key all the details, including FWD server and target appserver):
  • if a connection is already cached, it will be used
  • if the connection to the FWD server is lost, then discard that connection from the cache
  • when establishing the connection to the FWD server, it will access an API to get the (default) number of max available concurrent requests for the remote OpenClient: this will be determined by the FWD server, lets say 80% of the Dispatcher threads, always ensuring a minimum of two 'reserved' Dispatcher threads. It can be overwritten by some setting when/before executing the connectRemote call. This also needs to keep in mind that connections to different appservers can be established from the remote app (app1 and app2).
• the instance returned by LegacyJavaAppserverClient.connectRemote will:
  • use the cached connection
  • any call will be posted to a queue, from which a pool of workers (or an executor, doesn't matter), will pick up tasks from the queue and execute them. This pool of workers needs to be global for that FWD server connection (not per-appserver connection); the reason - multiple appserver connections to the same FWD server share the same pool of Dispatcher threads on the server side.
  • the call will block until the work is executed.

The existing RemoteLegacyJavaAppserverClient and RemoteAppServerConnectionPool can be refactored as needed. Only the LegacyJavaAppserverClient APIs like connectRemote and others need to be preserved.

• reuse the AppServerHelper instance for all calls on the same FWD server and appserver.
• limit the number of concurrent requests to the FWD server (not the appserver!).

Greg: why is BaseSession.listeners a LinkedList instead of a HashSet? Are the listeners dependent on when their order?

Yes, the intention was to honor them in order of registration.

Constantin Asofiei wrote:

• with its size limited to 80% of the number of Dispatcher threads

I think we need to make this configurable on the appserver. There can be multiple appservers exposed via OpenClient, with multiple applications connecting to them remotely... and allowing each of them 80% doesn't look right.

Instead, the appserver configuration can set the number of threads the remote side can use, and this can default to 80% of Dispatcher threads.

Perhaps we should have a safer way to set the number of dispatcher threads. For example, doesn't the number of agents/sessions tell us something about the number of dispatcher threads which are needed? I'd like our code to be less fragile in this regard, to the degree that we can have a smart calculation.

Greg Shah wrote:

Greg: why is BaseSession.listeners a LinkedList instead of a HashSet? Are the listeners dependent on when their order?

Yes, the intention was to honor them in order of registration.

OK, I'll use a LinkedHashSet instead, and add a comment that the order of registration is important.

Greg Shah wrote:

Perhaps we should have a safer way to set the number of dispatcher threads. For example, doesn't the number of agents/sessions tell us something about the number of dispatcher threads which are needed? I'd like our code to be less fragile in this regard, to the degree that we can have a smart calculation.

Actually, don't we queue the Dispatcher requests already? I mean, if all Dispatcher threads are busy, FWD will queue incoming requests, right? If so, then yes, we can calculate the number from the appserver configurations (max_agents for classic, max_agents * max_connections for multi-session, etc).

I don't know why I remembered that the dispatcher threads are the limit on the FWD server request.

Otoh, I don't think we want to reserve dispatcher threads for all apps, even if they don't use remote openclient.

Galya: when you have time, please review.

Code Review Task branch 9804a Revisions 15812 and 15813

1. In RemoteAppServerConnectionPool.dispatch() the javadoc states "In this mode, the request is processed immediately, on the same thread as the requester.". The request may block waiting on the Future but it is not processed on the same thread. Or am I misunderstanding?

2. The AppServerHelper is not thread safe for some of its state. I'm especially thinking about the disconnected member. I feel like we've discussed this before. With the current changes, I think it is even less obvious whether or not an instance can be accessed from more than one thread simultaneously.

3. In a separate task, I'd like to organize the code so that the remote client code is isolated in its own package and classes. Today the code is mixed together in a way that is hard to reason about. Where that code needs to share common code, factoring that into base classes that are more obviously run on both sides would be part of that work.

Greg Shah wrote:

Code Review Task branch 9804a Revisions 15812 and 15813

1. In RemoteAppServerConnectionPool.dispatch() the javadoc states "In this mode, the request is processed immediately, on the same thread as the requester.". The request may block waiting on the Future but it is not processed on the same thread. Or am I misunderstanding?

No, you are right. I've fixed the javadoc.

2. The AppServerHelper is not thread safe for some of its state. I'm especially thinking about the disconnected member. I feel like we've discussed this before. With the current changes, I think it is even less obvious whether or not an instance can be accessed from more than one thread simultaneously.

The AppServerHelper is not shared - only the AppServerEntry (the connection to the FWD server). The FWD OpenClient still uses connect/disconnect for each request (this is the connection to the appserver, not the physical network connection to the FWD server).

Please see rev 15814.

In BaseSession the change of LinkedList to LinkedHashSet requires to go over all implementations of SessionListener that can be added to the Set and confirm their hashcode and equals are well generated.

If this is not urgent, can we review and merge it after 8661b, because it's on the finishing line.

Galya B wrote:

In BaseSession the change of LinkedList to LinkedHashSet requires to go over all implementations of SessionListener that can be added to the Set and confirm their hashcode and equals are well generated.

Thanks, I've checked and all are OK. We already used contains to not add the same listener instance twice.

If this is not urgent, can we review and merge it after 8661b, because it's on the finishing line.

I think merge to trunk can be delayed. I'll check 7156c how it works, we will need to patch this branch anyway.

2. The AppServerHelper is not thread safe for some of its state. I'm especially thinking about the disconnected member. I feel like we've discussed this before. With the current changes, I think it is even less obvious whether or not an instance can be accessed from more than one thread simultaneously.

The AppServerHelper is not shared - only the AppServerEntry (the connection to the FWD server). The FWD OpenClient still uses connect/disconnect for each request (this is the connection to the appserver, not the physical network connection to the FWD server).

Please add some text to the AppServerHelper to that effect. Make it clear that the class is not thread safe and must not be used from multiple threads.

Greg Shah wrote:

Please add some text to the AppServerHelper to that effect. Make it clear that the class is not thread safe and must not be used from multiple threads.

Please see 9804a rev 15816.

7156c + 8904a patch worked, I'm waiting for some testing from Danut.

Testing went well over the weekend. I only emailed Constantin this morning and forgot to update the task.

• Remove pasoe references in RemoteLegacyServiceWorker and there seems to be one reference left in the javadoc of MultiSessionAppserver.runInSessionContext, please remove it too.
• I wonder if it's fine to keep the thread reference in RemoteLegacyServiceWorker, what about using WeakReference? What is the thread where LegacyJavaAppserverClient.connectRemote is called and can you be sure the worker object is removed from references before the attempt to sweep the thread?
• What about synchronizing LegacyServiceWorker.getHelper and RemoteLegacyServiceWorker.getHelper. I see the instantiation of the helper is called on pool initialization, so there should not be concurrent access that creates two instances, but not so sure about the case with isConnected being false.
• In RemoteLegacyServiceWorker.stop the javadoc hints that worker thread will be interrupted even if it's not the current one, but the code doesn't do it.
• RemoteServerInfo - fix the module name in the file header.
• As for BaseSession.listeners if the SessionListener implementations don't have the hashcode and contains methods overridden, it's simply comparing references and there is no benefit to having set instead of arraylist, especially having the contains check. I'm just not sure what was the idea behind the change.

Galya B wrote:

Review of 9804a r15848:

• Remove pasoe references in RemoteLegacyServiceWorker and there seems to be one reference left in the javadoc of MultiSessionAppserver.runInSessionContext, please remove it too.

Done.

• I wonder if it's fine to keep the thread reference in RemoteLegacyServiceWorker, what about using WeakReference? What is the thread where LegacyJavaAppserverClient.connectRemote is called and can you be sure the worker object is removed from references before the attempt to sweep the thread?

This would be the thread from a remote app (like a tomcat web app) which executes the i.e. web request. Using a WeakReference seems right. I'm not sure yet if is still needed or not.

• What about synchronizing LegacyServiceWorker.getHelper and RemoteLegacyServiceWorker.getHelper. I see the instantiation of the helper is called on pool initialization, so there should not be concurrent access that creates two instances, but not so sure about the case with isConnected being false.

• The instance returned by connectRemote is not meant to be shared between threads; so no need to synchronize RemoteLegacyServiceWorker.getHelper.
• LegacyServiceWorker.getHelper - each worker is on its own thread, a worker can't execute tasks outside of its thread. I think we are OK.

• In RemoteLegacyServiceWorker.stop the javadoc hints that worker thread will be interrupted even if it's not the current one, but the code doesn't do it.

I think the current implementation of RemoteLegacyServiceWorker.stop() is incorrect; it should terminate the Future obtained via executor.submit() - there should be only one active at a time.

I need also to test the timeouts.

• RemoteServerInfo - fix the module name in the file header.

Done.

• As for BaseSession.listeners if the SessionListener implementations don't have the hashcode and contains methods overridden, it's simply comparing references and there is no benefit to having set instead of arraylist, especially having the contains check. I'm just not sure what was the idea behind the change.

The difference is that a ArrayList.contains is O(n) while Set.contains is O(1). The performance issues could be seen in trunk before 9804a, as each connectRemote established a physical network connection with the FWD server.

Changes are not yet committed.

Constantin Asofiei wrote:

• What about synchronizing LegacyServiceWorker.getHelper and RemoteLegacyServiceWorker.getHelper. I see the instantiation of the helper is called on pool initialization, so there should not be concurrent access that creates two instances, but not so sure about the case with isConnected being false.

• The instance returned by connectRemote is not meant to be shared between threads; so no need to synchronize RemoteLegacyServiceWorker.getHelper.
• LegacyServiceWorker.getHelper - each worker is on its own thread, a worker can't execute tasks outside of its thread. I think we are OK.

Aren't the workers supposed to be reused by the SoapHandler and RestHandler? I mean worker.getHelper() seems to be called on the jetty threads.

• In RemoteLegacyServiceWorker.stop the javadoc hints that worker thread will be interrupted even if it's not the current one, but the code doesn't do it.

I think the current implementation of RemoteLegacyServiceWorker.stop() is incorrect; it should terminate the Future obtained via executor.submit() - there should be only one active at a time.

If the future is cancelled, it won't stop the worker thread from what I've seen implementing MSA, so in MSA I did catch the timeout and cancel exceptions and called interrupt on the worker thread.

The difference is that a ArrayList.contains is O(n) while Set.contains is O(1).

This is true if the objects have properly implemented hash codes, or none of them have any hashcode overwritten (in the second case it will compare references which is wild to do with sets in my opinion, but I guess if it works for performance, let's not get concerned about the details).

Galya B wrote:

Aren't the workers supposed to be reused by the SoapHandler and RestHandler? I mean worker.getHelper() seems to be called on the jetty threads.

No, all getHelper() calls (for the non-remote pool case) are executed inside the 'work lambda' which is posted to the worker. I can't find any case where it would be executed on a jetty thread - and if such cases exist, then that is wrong.

If the future is cancelled, it won't stop the worker thread from what I've seen implementing MSA, so in MSA I did catch the timeout and cancel exceptions and called interrupt on the worker thread.

Hmm... thanks, I need to see how I can interrupt the thread from the executor pool then.

The difference is that a ArrayList.contains is O(n) while Set.contains is O(1).

This is true if the objects have properly implemented hash codes, or none of them have any hashcode overwritten.

No implementation has hashCode overrides - we are OK.

Galya, please see 9804a rev 15849 - this address the review.

I've added timeout for the remote-side request.

For the 'stop' and 'terminateWorkers' on the remote side: this is actually a no-op; the worker in this case has no resource to clean up.

Constantin Asofiei wrote:

Galya, please see 9804a rev 15849 - this address the review.

I've added timeout for the remote-side request.

For the 'stop' and 'terminateWorkers' on the remote side: this is actually a no-op; the worker in this case has no resource to clean up.

It's ok. My only problem is with the timeout. There is the default timeout in pasoe, then the custom timeout the client sets for the server invocation, and now a third timeout on the client side. The weird behaviors that can come up from the mismatching of those values can be weird...

Branch 9804a was merged to trunk rev 15845 and archived.