grrolland / ngx-distributed-shm Goto Github PK

Hi,
Currently, there is no dependency injection, dependencies are instanciates in classes who needs them.
The main drawback : writing unit test is not easy, you will need PowerMock, mock constructor call, and maybe mock static method too

Integrating a minimalist DI framework can solve this.

This article talk about Micronaut DI integration with vertx : https://taraskohut.medium.com/vert-x-micronaut-do-we-need-dependency-injection-in-the-microservices-world-84e43b3b228e

when calling the "set" command twice with the lua script we have an issue with a ERROR malformed_request that is returned by the dshm server.
Here is a dump of the buffer received by the protocol handler

buffer:set sessions::ZWH6NJT2o6hYROBZqaS1eA 3600 22
buffer:tpHMMfyybPjneoCl30YYyQ
buffer:
buffer:set sessions::ZWH6NJT2o6hYROBZqaS1eA 3600 192
buffer:V0ckHpTdetg42-cKciJHjGBAcomwZcrpYfV71e-z4f49PfHZJnDAatMnPh-Ud2I4e8JA0TGv_DfkTLrTULiqdt8aIzcB_PmPHI0f2xp0JvTSY7PKbglLf0pqmyqVZbCqs3dDO-br4Xp03LwoWl8V_ZHMzguMRoDaEmvj7rBvjblbRcj--5Dden6_FfDUolhY
buffer:

You can see there is a call to the handle with an empty buffer when setting a value and this call will generate an ERROR malformed_request string on the socket.
The reason for this is that the lua script does not respect the protocol and adds an \r\n at the end of the value, see here. But the protocol is switching to fixedSizeMode thus making the last \r\n part of the next buffer handling.
The fix is simple and is working for us that is removing this line and I'll propose a PR soon.

Hi,

When deployed in a cluster with backup-count greater than 0, IncrCommand is executed on each backup. For example, with 1 backup;

INCR KEY 1 1
LEN 1
1
DONE
GET KEY
LEN 1
3
DONE

The issue is related to this line in IncrCommand :

map.set(entry.getKey(), new ShmValue(newval, expire), expire, TimeUnit.SECONDS);

This code fix the issue :

ExtendedMapEntry<String, ShmValue> extendedMapEntry = (ExtendedMapEntry<String, ShmValue>) entry
extendedMapEntry.setValue(new ShmValue(newval, expire), expire, TimeUnit.SECONDS);

This is documented in Javadoc :
The entry's value will be null, if the entry for the key doesn't exist. You can create new mapping by setting a non-null value or remove existing mapping entry by setting the value to null.

Hi,

When testing SET command with telnet, i get an malformed_request

SET KEY 0 1
1
LEN 1
1
DONE
ERROR malformed_request

In unit test, we don't have the malformed_request, because "\r\n" is not send

getWriter().write("SET region:key 1 10\r\n");
getWriter().write("1234567890");
getWriter().flush();

Maybe there is a way to send the command without carriage return in telnet ?

Otherwhise, this issue can be fixed in two ways

1. Setting an expectedMode to FrameMode.END

 else if (expectedMode == FrameMode.DATA) {
       final String result = currentCommand.executeDataPart(buffer.toString(PROTOCOL_ENCODING));
       socket.write(result, PROTOCOL_ENCODING);
       expectedMode = FrameMode.END;
       parser.delimitedMode(PROTOCOL_DELIMITER);
  }
  else {
        expectedMode = FrameMode.COMMAND;
   }

but this will make a breaking change

2. Handling empty command in ShmProtocolHandler.handle :
   if (expectedMode == FrameMode.COMMAND && !buffer.toString(PROTOCOL_ENCODING).isEmpty()) {
   }
   but not sure allowing empty command is a good idea

Hello,
We are thinking of using dshm with openresty in order to cache our web app sessions among multiple instances in a Kubernetes environment.
You provide some Kubernetes manifest example which is nice but in terms of production readyness I was wondering if you could provide some guidance in the RAM requirements for the container (Requests and limits).
I would also like to ask if it is possible to setup some kind of readyness and liveness probe as well ?

Current base image is debian-buster which is vulnerable to CVE-2021-33574.

Hi,

The lua.shared semantic has evolved. The method incr now accept an optional parameter init_ttl :

syntax: newval, err, forcible? = ngx.shared.DICT:incr(key, value, init?, init_ttl?)

init_ttl allow to create the missing key with a ttl and avoid to make another call to handle expiration (incr than expire)

remaining, err = dict:incr(key, -1, limit, window)

I can submit a PR to add this new optional parameter in ngx-distributed-shm

Hello,
We have deactivated the stopping of the sig term in this PR. And now the signal reaches the JVM and hazelcast performs a gracefull shutdown.
But the process does not end and the pod remains running until K8s timesout and kills the container.
That would be nice to have a proper end of container and will make the updates faster in K8s.
I did not find a way to fix this :(

Hi,

There is an issue with the IncrCommand and deadline/getLastingTime() calculation.

The problem is related to a double to int conversion.

Each time a key is incremented, the delay will decreases faster than time passes due to the conversion

With 200 command

SET KEY 60 4
1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
INCR KEY 1 1000
....

The output will look like :

...
1091
DONE
LEN 4
1092
DONE
LEN 4
1001
DONE
LEN 4
1002
DONE
LEN 4
1003

Hazelcast has a method setValueWithoutChangingExpiryTime that can be used instead of managing the lasting time.