search

pharo-nosql / mongotalk

A Pharo driver for MongoDB
MIT License
19 stars 13 forks source link

Conditional support for TLS #91

Closed rydnr closed 2 years ago

rydnr commented 3 years ago

This PR is a trivial change to add an optional "tls" parameter to the Mongo class. If omitted, it behaves as before. It set to true, the socket class is ZdcSecureSocketStream instead, and sets the server name to comply with SNI.

I've tested it with MongoDB Atlas.

rydnr commented 3 years ago

I didn't test TCP_NODELAY with TLS. I tried to be as non-disruptive as possible with existing codebase.

zecke commented 3 years ago

We want nagle and other things disabled for sure. I don't know if zinc is doing this already.

tinchodias commented 3 years ago

Thanks. The wiki entry for Nagle mentions TCP_NODELAY: https://en.wikipedia.org/wiki/Nagle%27s_algorithm

tinchodias commented 3 years ago

I didn't do this before: I searched for occurrences of TCP_NODELAY in the source code in my image with Voyage loaded, and it's not only Mongo:

ZnNetworkingUtils>>
setServerSocketOptions: socket
    socket
        setOption: 'TCP_NODELAY' value: 1;
        setOption: 'SO_SNDBUF' value: self class socketBufferSize;
        setOption: 'SO_RCVBUF' value: self class socketBufferSize

and socketBufferSize is defined as 4096.

I see that when a Zinc server is started, the ZnSingleThreadedServer>>#initializeServerSocket uses the setServerSocketOptions: to enable the TCP_NODELAY option.

zecke commented 3 years ago

Yes but that's the server side. We probably want this for the client as well.

noha commented 3 years ago

Is there anything to do about this?

zecke commented 3 years ago

Is there anything to do about this?

I think we should be explicit and switch off Nagel. I don't know whether Zinc is doing this for us in the client.

noha commented 2 years ago

I'm closing this for now because I don't understand what was the problem. If the problem persist please reopen