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

Feature Request: make it work well when several instances are run concurrently #31

Open
expipiplus1 opened this issue May 5, 2021 · 17 comments
Labels
wontfix This will not be worked on

Comments

@expipiplus1
Copy link

for i in $(seq 1 10); do matrix-commander --message $i & ; done

Running the above yields:

  • One successful message to the channel
  • Two peewee.OperationalError: database is locked errors
  • Six corrupt messages in the channel Unable to decrypt: The secure channel with the sender was corrupted
  • One ERROR: matrix-commander: Message send failed. Sorry. with no traceback

It would be excellent if multiple instances of this program could run concurrently, or at the very least wait politely for a while for any siblings to finish.

@8go
Copy link
Owner

8go commented May 20, 2021

Thank you @expipiplus1 for bringing this to my attention.

Yes, you are right. Concurrency is currently not supported and will cause errors and problems.

Making the matrix-commander concurrent would be quite some work.

Just as a workaround, for performance reason it is much better anyway to first prepare all messages and send all N messages with a single matrix-commander command.

Like:

# some preprocessing, preparing all N messages
# calling matrix-commander similar to this:
matrix-commander --message $M1 --message $M2 ... --message $Mn
# this requires less resources and is way faster than calling matrix-commander N times

@8go 8go closed this as completed May 20, 2021
@expipiplus1
Copy link
Author

expipiplus1 commented May 21, 2021 via email

@8go
Copy link
Owner

8go commented May 25, 2021

Sure, a "lockfile" or a PID-file is possible, maybe in combination with a flag/argument --wait which can be used as:

  • if --wait is not set and another MC process is running, the newly invoked MC process simply aborts with a message "another MC already running"
  • if --wait is set and another MC process is running, the newly invoked MC process will wait (sleep) and periodically (e.g. every 1 or 2 seconds) check if the lockfile (PID file) is still there and by doing so wait until other MC instances have finished, in which case it will itself create the lockfile (PID file) and start running.

From the 2 variants lock-file vs PID-file the PID-file logic is already implemented.

So, overall what you want is possible. The missing part, logic, is the "check, sleep, recheck" logic.

It is not high on my priority list. Any nice soul out there that wants to contribute this feature as a PR?

@8go 8go changed the title Doesn't work very well when several instances are run concurrently Feature Request: make it work well when several instances are run concurrently May 25, 2021
@8go
Copy link
Owner

8go commented May 25, 2021

Reopened issue as Feature Request in hope to find a contributor for this feature.

@8go 8go reopened this May 25, 2021
@expipiplus1
Copy link
Author

Thanks for reopening. I wonder if there's some existing program which wraps other programs doing this lock file management. Probably wouldn't take too long to do in bash. Next time I bump into it I might take a look.

@8go
Copy link
Owner

8go commented May 25, 2021

Thanks for reopening. I wonder if there's some existing program which wraps other programs doing this lock file management. Probably wouldn't take too long to do in bash. Next time I bump into it I might take a look.

Yes, it could also be implemented OUTSIDE of matrix-commander as you point out correctly. You are right, a bash script wrapper could look for the presence of a PID file in the PID directory (e.g. /home/$USER/.run) and "pause" until no more PID files are in the directory.

Only a 99.9% solution because this would still allow 2 MC instances to start concurrently, both check at the same time, both find the dir empty of PID files, both start, .... and then possibly collide. A race condition.

@expipiplus1
Copy link
Author

expipiplus1 commented May 25, 2021 via email

@nirgal
Copy link
Contributor

nirgal commented Aug 31, 2021

@expipiplus1 As a workaround, you could wrap your calls with flock.
I mean rather than run "matrix-commander args" run instead "flock -c mylockfile matrix-commander args"...
This will delay the execution until the lock is available

@expipiplus1
Copy link
Author

expipiplus1 commented Aug 31, 2021 via email

@8go 8go added the wontfix This will not be worked on label Jun 1, 2022
@xurizaemon
Copy link
Contributor

Another tactic would be to run parallel instances of matrix-commander in containers, eg via docker matrixcommander/matrix-commander. Containers could be given whatever shared state is required via a shared mount, and at the same time isolated from interfering with each others running.

@8go
Copy link
Owner

8go commented Oct 2, 2022

Thank you all @nirgal @expipiplus1 @xurizaemon for adding your excellent ideas. Please keep sharing. So, others can learn and take some clues, or even implement some of your suggestions.

For other people: Keep it going and please keep adding more suggestions if you have any! They are welcome! Thanks.

@8go
Copy link
Owner

8go commented Oct 11, 2022

See https://github.com/8go/matrix-commander-rs

An idea to improve performance 😄

Maybe Rust also allows for more concurrency.

Talk about it to your friends, post about it, spread the word and provide PRs. 🙏

And give it a ⭐ if you like the idea.

@edwinsage
Copy link
Contributor

If running concurrently is a goal (for example to speed up making numerous requests), you should also be able to achieve it by creating multiple credentials/stores and having each process use a different one, as described here:
#86 (comment)

@8go
Copy link
Owner

8go commented Nov 21, 2022

If running concurrently is a goal (for example to speed up making numerous requests), you should also be able to achieve it by creating multiple credentials/stores and having each process use a different one, as described here:
#86 (comment)

Correct. Should be working today.

@Benjamin-Loison
Copy link
Contributor

Benjamin-Loison commented Jun 12, 2023

I personally made sure to have a single matrix-commander installed with:

sudo find / -type f -name 'matrix-commander' 2> /dev/null

Then I renamed the given matrix-commander to matrix-commander_without_flock and I've added the following matrix-commander with +x permission:

#!/bin/bash

flock /var/lock/matrix-commander "$( cd "$( dirname "$0" )" && pwd )"/matrix-commander_without_flock "$@"

Thanks to @nirgal comment for using flock.
That way I don't modify my scripts but only the matrix-commander program.
After this change @expipiplus1 command line works fine (after adding parenthesis around matrix-commander --message $i & otherwise I get bash: syntax error near unexpected token `;').

Note that Docker still suffer this issue and I'm currently unable to make flock work as wanted with Docker.
My approach was to change for something like this:

-ENTRYPOINT ["/bin/python3", "/app/matrix_commander/matrix-commander" ,"-s", "/data/store", "-c", "/data/credentials.json"]
+ENTRYPOINT ["/bin/flock", "/dev/shm/matrix-commander", "/bin/python3", "/app/matrix_commander/matrix-commander" ,"-s", "/data/store", "-c", "/data/credentials.json"]

in docker/Dockerfile.

However a host level flock, as done above, solves the concurrency issue with:

#!/bin/bash

flock /var/lock/matrix-commander docker run --rm -tv /my/chosen/folder:/data:z matrix-commander "$@"

@8go
Copy link
Owner

8go commented Oct 10, 2023

Thank you @Benjamin-Loison for sharing this with everyone.

@jotwg
Copy link

jotwg commented Feb 21, 2024

I'm using 'task spooler' (https://manpages.ubuntu.com/manpages/xenial/man1/tsp.1.html) to queue matrix-commander invocations. Works pretty good for sending notifications from my icinga2 monitoring system via matrix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

7 participants