Reconsider ScanClient.py singleton

[kasemir]

Why is ScanClient a singleton? Scan server allows multiple clients. It's perfectly fine to run multiple clients which concurrently submit scans. Right now CSS will always use a new Jython context, but as a result of optimization it might re-use the same Jython context, where each 'Action' creates its own ScanClient. Why should that be prohibited?

[ksdj]

This is because in the original design,I just want to control the number connection objects. The number of clients should be equal to the number of ScanServer addresses users want to use synchronously. But I don't know whether it's necessary. So I just made it a singleton for , maybe ,in the future , it can be implemented to a 'Connection Pool' or something. So I just remove the singleton? What do you think?

[kasemir]

Note that this is HTTP (REST). There may not be any permanent connection at all, because HTTP is stateless. Each GET http://scan.server.host:4810/whatever will 1) Establish a TCP connection 2) Send the request 3) Read the reply 4) Close the TCP connection

So technically, there is no need to pool anything at all. The server doesn't care about the number of clients, and each client tends to connect/disconnect for each request.

The internal network details could of course change. Have in fact changed. The original scan server had a Java RMI interface. Now it's REST. In the future there could be another mechanism like JMS. So it's certainly a good practice for client code to create one ScanClient() instance and then use that, because in the future that ScanClient() instance may somehow establish a longer lasting TCP connection to the scan server, or for example start a thread to periodically fetch updates, and you want to limit the number of such threads.

So enforcing the singleton is not necessary, and in case it hurts us, we can at any time remove that constraint. At the same time, we can also leave it in for now as long as it doesn't hurt us.

Maybe we for now just add a comment to the ScanClient

       '''   
        Singleton method to make sure there is only one instance alive.

        The singleton is not strictly necessary, see #3, but for now in place
        to encourage client code to create a single ScanClient() instance,
        because that may allow for future optimizations.
        '''

[kasemir]

There is one problem with the actual singleton implementation.

Assume you do want to communicate with 2 different scan servers:

scan1 = ScanClient('host1')
scan2 = ScanClient('host2')

The current singleton implementation doesn't check for the host or port, it simply returns the instance that was created first, so scan2 == scan1, and then updates the URL, so now scan1 actually talks to host2.

Maybe it is best to remove that singleton check, because it's not necessary and in fact even adds a bug?

[shengb]

Agree. there is no reason why we should use this singleton. Especially, even the user want to create many scan objects for any reason, we should not limit it. If we really want to have a singleton method, we should do it as single object for a host + port.

[ksdj]

-- The server doesn't care about the number of clients, and each client tends to connect/disconnect for each request. --we should do it as single object for a host + port. Agree with you all. I will remove the singleton implementation, and leave there the comments.