vouch-opensource / krell

Simple ClojureScript React Native Tooling
Eclipse Public License 1.0
675 stars 37 forks source link

Ensuring write.socket ordering on android #101

Closed olivergeorge closed 3 years ago

olivergeorge commented 4 years ago

In investigating #100 I found a way to ensure the write.socket order on android.

The specific problem is known and there's a workaround. I suspect that workaround is buggy (see #100) and that there's a simpler solution.

The problem is that loadFile writes via socket.write(payload); don't always appear on the server in order. (I demonstrated this by giving requests an incrementing id and checking the order on the server side.)

socket.write provides a callback which is invoked after the payload is really written. So waiting for that before doing other socket.write calls will ensure order.

Here's some prototype code demonstrating the fix.

var loadFileSocket = null;
var writeQueue = [];
var writeLoopActive = false;

const writeLoop = () => {
    if (!writeLoopActive && writeQueue.length > 0) {
        writeLoopActive = true;
        payload = writeQueue.shift()
        loadFileSocket.write(payload, undefined, () => {
            writeLoopActive = false;
            writeLoop();
        });
    }
}

const socketWrite = (payload) => {
    writeQueue.push(payload);
    writeLoop();
}

const loadFile = (path) => {
    let req = {
            type: "load-file",
            value: path
        },
        payload = JSON.stringify(req)+"\0";
        socketWrite(payload);
};

Various other android specific code could be removed or refactored to use this approach.

olivergeorge commented 4 years ago

Note: I upgraded react-native-tcp-socket to 4.2.0 for my app. Might not be necessary.

olivergeorge commented 4 years ago

In case it's useful to see a full set of changes associated with the prototype:

https://github.com/vouch-opensource/krell/compare/master...condense:krell-101?expand=1

swannodette commented 4 years ago

Noting that we already have a write queue for Android - it could probably be made simpler, yes, but it's not clear yet there's any issues with the existing order enforcement that the above fixes.

swannodette commented 3 years ago

This one does not matter anymore - there are no queues for files loads, everything is done via Metro now.