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

Queue messages #21

Open
fregante opened this issue Sep 24, 2021 · 0 comments
Open

Queue messages #21

fregante opened this issue Sep 24, 2021 · 0 comments
Labels
question Further information is requested

Comments

@fregante
Copy link
Contributor

fregante commented Sep 24, 2021

Follows:

Regarding queues, which have not been implemented, they'd block every message until a previous message has responded, which in practice would slow down the application at a minimum and cause pileups in worst case.

The simplest of response (ACK) takes about 100ms (I've seen this before), so we could only ever send 10 messages per second:

|---RETRY---ACK . . . . . . Response|
               |---ACK . . . . . . Response|
                      |---ACK . . . . . . Response|
                             |---ACK . . . . . . Response|

while a non-queued version (what we have) is able to just queue the messages in the browser, which can then respond as necessary:

// Retries are slightly randomized with https://github.com/tim-kos/node-retry
|---RETRY. . . . . . Response|
|----RETRY . . . . . . . Response|
|--RETRY . . . . . . . Response|
|---RETRY . . . . . . . . . Response|
|----RETRY . . . . . . . . Response|

If the application’s responses benefit from or require a certain order of unrelated messages, that's a code smell to me. I think ordering is best handled where necessary.


Something I could improve though is to stop the retries if a message is currently being retried:

|---RETRY. . . . . . Response|

// Then other calls are attempted while waiting
// but they all start once the first retry is done
  |------RETRY . . . . . . . Response|
   |-----RETRY . . . . . . . . Response|
    |----RETRY . . . . . . . . . Response|
    |----RETRY . . . . . . . . Response|

But I don't know whether this is worth it either.


I'll try porting this file to Messenger since I saw this explanation:
https://github.com/pixiebrix/pixiebrix-extension/blob/0fbacfedb1e0eee40d76a2168e6e4cb0b67711c5/src/background/browserAction.ts#L149-L157

@fregante fregante added the question Further information is requested label Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

1 participant