Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom id_key function #29

Open
dagadbm opened this issue Dec 6, 2021 · 20 comments
Open

Custom id_key function #29

dagadbm opened this issue Dec 6, 2021 · 20 comments

Comments

@dagadbm
Copy link

dagadbm commented Dec 6, 2021

Hey!

I am trying to use your framework but I already have my own custom id_key function which I NEED to use.

I basically use a spellchecker!

Imagine this flow:

text change #1 => websocket id_key 1
text change #2 => websocket id_key 2

websocket id_key 2 response
websocket id_key 1 response

Without any ability to manage my id_key the id_key 1 response is what is going to be saved on the interface. Which is wrong because it is outdated. It happens a lot of times that the server is slow and unpredictable to respond.

Therefore I need a way to discard "old" responses that I dont want.

I did this before by creating my own id_key with a simple JS date.toString().

But it can get even more wacky and crazy because I have several independent sentences that have different errors like so:

text change #1 on sentence #1 => websocket id_key 11
text change #2 on sentence #1 => websocket id_key 21

text change #1 on sentence #2 => websocket id_key 12

websocket id_key 21 response => i want this response
websocket id_key 12 response => i also want this one because it is from a different sentence

So I need a way to manage this complexity, and the way I do this is to have my very own custom identifier that i send to the server so that I can compare.

With your framework I need to use your message_id value that comes from the encode function and I have no way of managing this.

Can you help me out here?
Do you have any suggestions or alternatives that could help me with my use case?

TLDR: How can I ignore specific request responses based that are "older" that I no longer want to act on their response.

Im willing to talk a bit more if you think this kind of problem is interesting to you or If I am completely misunderstanding your framework.

@houd1ni
Copy link
Owner

houd1ni commented Dec 6, 2021

Hi, David! Thank you for the good issue!

If I understand well, it can be accomplished by either by

  • Adding some timestamps to messages to be echoed from that server side or
  • By saving some messages signature like its' own hash that identify them in a response.

Also, I'd say that the API for this is quite full and would probably become over-complex if add some ability to identify a message that was sent that way. It's now just await to get a response, and when somebody is able to perform those to options, then this API stays that pretty look. By other side, we have customisations and ability to add custom events to plain WS instance and use it as needed. Though, I'll think is a flag like complex_responses and const { id, message, data, ...stuff } = await wsp.send(my_data) could be handy, but for now I see no other usecases except for one with totally no access to backend and its' responses. Any idea ?

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

Hey.

I did a smart workaround by playing with the encode/decode functions and adding a string with _ that I can then split.

My API has 2 methods: full checks (super slow) and partial checks (kind of slow)

I have a class that has this variable:

    this.lastCallId = {
      full: {},
      partial: {},
    };

my encode/decode has this logic:

      server: {
        id_key: 'wspId',
        data_key: 'wspData',
      },
      encode: (messageId, data) => {
        const uid = data.check.uid;
        const { type, id, timestamp } = this._parseLastCallId(uid);
        const uidWithWspId = `${type}_${messageId}_${id}_${timestamp}`;
        data.check.uid = uidWithWspId;

        return JSON.stringify(data);
      },
      decode: (rawMessage) => {
        const decoded = JSON.parse(rawMessage);
        const uid = decoded.data.uid;
        const { wspId } = this._parseLastCallId(uid);
        Object.assign(decoded, {
          wspData: decoded.data,
          wspId,
        });

        return decoded;
      },

And i have these helper functions:

  _parseLastCallId(uid) {
    const [type, wspId, id, timestamp] = uid.split('_');
    return {
      type,
      wspId,
      id,
      timestamp,
    };
  }

  _isLastCallId(uid) {
    const { type, id, timestamp: uidTimestamp } = this._parseLastCallId(uid);
    const lastCallId = this.lastCallId[type][id];
    const { timestamp: lastCallIdTimestamp } = this._parseLastCallId(lastCallId);
    return uidTimestamp === lastCallIdTimestamp;
  }

  _generateFullCheckId(taskId) {
    const timestamp = Date.now().toString();
    const id = `full_wspId_${taskId}_${timestamp}`;
    this.lastCallId.full[taskId] = id;
    return id;
  }

  _generatePartialCheckId(segmentIndex) {
    const timestamp = Date.now().toString();
    const id = `partial_wspId_${segmentIndex}_${timestamp}`;
    this.lastCallId.partial[segmentIndex] = id;
    return id;
  }

Then on the await of the send i do this logic:

    try {
      const data = await this.wsp.send(message);
      // Discard if response is not last requested
      if (!this._isLastCallId(data.uid)) {
        return null;
      }
      return data;
    } catch (e) {
      return null;
    }

This is working.

The hardest part of this is that your encode/decode and send logic really MUST have the packNumbers id you generate so I had to come up with a way to keep that value on the decode else I would never get the data back (and you have no logging in these cases so I had to understand your source code to see what was wrong).

I have another question though:

Is it possible to get a specific timeout per send request, and not just a global timeout.

Also I am not sure if this is a problem with your framework or my backend server, but its seems that he always replies to the requests sequentially. But your code should work regardless right?
Imagine sent: req1, req2, req3. received: req2, req3, req1.
your code allows for this out of the box right?

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

There is something wrong with the connections which Im having a hard time understanding.

So this is the relevant part of my config:

      timeout: 30 * 10000, // in ms
      reconnect: null,
      lazy: true,
      ping: null,

Now check out these logs:

1 - sent request (very large payload)
2 - open event
3 - close event (literally immediately after the open event log)

a LOT of time passes and nothing happens.
I force a NEW request and this is what happens next:
4 - sent request (small payload)
5 - open connection
6 - message event from request #1 received (*)
7 - message event from request #2 received.

(*) This is the interesting part, I received the message from request #1
According to your code the time taken was: 139692 ms
The server however send metadata about the time and for the server it was: 22 s
Notice the big discrepancy here.
It seems like because the connection automagically closed (step 3) i never receive the other request.

I want to always receive the requests but I don't want to keep reconnecting when I don't have anything to send.
Can you help me figure out whats wrong here? Am I missing some key web socket spec information about this issue?

@houd1ni
Copy link
Owner

houd1ni commented Dec 7, 2021

It seems like because the connection automagically closed (step 3) i never receive the other request.
yes, because of ping: null in your config.

Also, it's interesting to debug this in network tab in developer tools to see actual messages and their timings.

So, you totally have no access to the server response ? To track them without any quirks.

The library works fully asynchronously: regardless of outgoing messages order, they stay and wait for their responses together without any order, but you can manually stop them by your code, sort of:

const data1 = await wsp.send(request1)
const data2 = await wsp.send(request2) // this won't work if previous is stuck.

The idea of customizing config per message is interesting. Gonna think of it today, - maybe will add it and let you know!

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

i cannot use ping because the server is not expecting an empty message.

The network tab for web sockets only shows the first request, everything else is then hidden.

I tried doing like this in the code:
ws.ready().then(() => ws.send())
and like this it worked.

Are you sending requests before the web socket being open? Because that can cause problems from what I read on stackoverflow.

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

if the connection closes (not by the user but just because)

image

i believe it should just automatically open it again.
Because I don't want to have to keep reconnecting when the connection is closed.
Why put that strain on the server?

I have some users that just decide to keep the page open for hours. But they are not really using anything.In that case doesn't it make sense to always reconnect right?

I hate web sockets.

@houd1ni
Copy link
Owner

houd1ni commented Dec 7, 2021

i cannot use ping because the server is not expecting an empty message.

The message can contain something. ping option can contain some string.

The network tab for web sockets only shows the first request, everything else is then hidden.

Weird browser behaviour. Try another or check settings etc. After click on a WS connection it should show self-refreshing list of messages.

.ready() is designed to wait when connection is established.

Are you sending requests before the web socket being open?

lazy option that you triggered is to avoid this without first ready() call and open connection only after first .send() call.

@houd1ni
Copy link
Owner

houd1ni commented Dec 7, 2021

if the connection closes (not by the user but just because)

If you triggered ping off, then browser closes connection by itself and we cannot do anything with that (but the library reconnects automatically then).
The english error message is thrown by this library. It literally explains that you have sent data in a closed socket. To place something in a queue after reopening, use await ws.ready() that should wait until it's reconnected. I'll also think why it does not do it automatically, - maybe add it also, if needed and let you know.

Also, I was a bit wrong about lazy: it just waits until first data is sent before establish a connection, nothing more, so I've edited the message.

Again about that case with messages tracking, you really have no access to server side to track them anyhow ?

Also this lib is designed to fall in 💘 with them c: They are really cool if used properly.

UPD: when ping is disabled, connection shuts down automatically by a browser in approx a minute as I stated in a first sentence here, hence your users won't stay connected for hours. That's OK. If you want to close it even earlier, use wsp.close().

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

But when the connection gets automatically disconnected. if i want to send a new request, shouldn't thee connection be automatically (by the framework) as well?

@houd1ni
Copy link
Owner

houd1ni commented Dec 7, 2021

But when the connection gets automatically disconnected. if i want to send a new request, shouldn't thee connection be automatically (by the framework) as well?

I'll also think why it does not do it automatically, - maybe add it also, if needed and let you know.

Use .ready() for now before such .send() calls. I'll let you know why it is such way or add autoreconnect for .send()s later today, but for now it explicitly won't do it.

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

Im trying to use ready but it doesnt work when the connection was closed (not by me)

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

I see now o my previous client, before every send he checks if the connection is closed, and if it is he creates a new one.

I can do something similar with a class getter and get the "wsp" instance and create a new one if the socket was closed

@houd1ni
Copy link
Owner

houd1ni commented Dec 7, 2021

Could you please tell what shows if write console.log(wspinstance.socket.readyState) into that place where .ready() does not work properly ?

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

image

it seems you probably delete the socket reference when an on close happeens?

There are two use cases here:

The API user really wants to close the connection.
The connection self closes but you want to use it afterwards.

I think its not so clear separation between these two use cases in the code and this is why this happens.

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

https://github.com/houd1ni/WebsocketPromisify/blob/master/src/connectLib.ts#L56

here you should only set this stuff to null if it was force_close to true right?

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

Im not sure if you saw this but the this.log('send') is not totally accurate,
this log point should move to when you are actually sending the message via the socket and not when you call the send method that: #30.

Thank you so much for taking your time in talking to me.

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

Also the reconnect() timeout really isnt doing anything.
I put a 1 minute timeout.

I was expecting the following flow:

close connection

  • wait 1 minute
  • open connection
    ....
    close connection
  • wait 1 minute
  • open connection

but in the code you are listening to the socket 'close' option and you automatically reconnect and then add a timeout. Which is a bit counterintuitive (to me at least) that I was expecting.

@houd1ni
Copy link
Owner

houd1ni commented Dec 7, 2021

it seems you probably delete the socket reference when an on close happeens?

If close happens, this means, close event has been triggered and ready() should work afterwards. To reproduce it, I need some snippet at least.

The API user really wants to close the connection.

It's too far from this issue topic, but why simply use .close() then ?

The connection self closes but you want to use it afterwards.

It will reconnect by itself. To be safe, just use .ready() for now.

I think its not so clear separation between these two use cases in the code and this is why this happens.

What is confusing exactly ?

here you should only set this stuff to null if it was force_close to true right?

What do you mean ?

but in the code you are listening to the socket 'close' option and you automatically reconnect and then add a timeout. Which is a bit counterintuitive (to me at least) that I was expecting.

You are right, - it's more like interval. Will change the corresponding documentation. Thanks! Btw, setting it to null disables automatic reconnections. Will better describe it.


So, we have some suggestions:

Reconnect by send() itself instead of manually check by ready().

Pros: more intuitive code.
Cons: Less predictible, less explicit ? What if we don't want to reconnect, instead use try/catch or something? Need to make some snippets before/after. Others ?

Make some configurable option that makes send() returning an object with additional data.

Pros: More flexibility, especially with third-party server side.
Cons: More complex API in other cases.

... something else ?

@dagadbm
Copy link
Author

dagadbm commented Dec 7, 2021

Reconnect by send() itself instead of manually check by ready().

Even by manually checking ready() it doesnt work.

=> send a message via websocket
=> wait until natural timeout from browser
=> send another message - you willl get an error, if you wait for ready() nothing happens because ready never returns.

Make some configurable option that makes send() returning an object with additional data.
I mean being able to edit the config options per each send request. not returning an object with additional data.

@houd1ni
Copy link
Owner

houd1ni commented Feb 28, 2023

This will be updated after #37 is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants