search

Z-Hub / Z-Push

Home of Z-Push
https://z-push.org
GNU Affero General Public License v3.0
94 stars 43 forks source link

Duplicate-Pings-With-Same-Timestamp #56

Closed liverpoolfc-fan closed 6 months ago

liverpoolfc-fan commented 8 months ago

Released under the GNU Affero General Public License (AGPL), version 3 and Trademark Additional Terms.

What does this implement/fix? Explain your changes.

This code detects when two or more Ping Requests from the same device/user have the same start timestamp and tells the ones with the smaller PID to self-terminate

Does this close any currently open issues?

Fixes issue #55

Any relevant logs, error output, etc?

Initial check-in includes DEBUG logs to demonstrate the functionality. These should be removed once the fix has been accepted. 

Line 1196: 16/02/2024 16:08:17 [1496456] [DEBUG] MultiPINGS: Other process [31200] starttime same as mine : Other process ID is "smaller" than mine - return FALSE
Line 1197: 16/02/2024 16:08:17 [1496456] [DEBUG] MultiPINGS: This is my PID - Ignore me - return FALSE
Line 1503: 16/02/2024 16:09:17 [31200] [DEBUG] MultiPINGS: This is my PID - Ignore me - return FALSE
Line 1504: 16/02/2024 16:09:17 [31200] [DEBUG] MultiPINGS: Other process [1496456] starttime same as mine : Other process ID is "bigger" than mine - return TRUE

Where has this been tested?

Server (please complete the following information):

Smartphone (please complete the following information):

matidau commented 8 months ago

Thanks @liverpoolfc-fan

Always appreciate your PRs

I won't have a chance to test this for the next couple of weeks. But will do it as soon as I can.

Cheers, Mat

grammmichi commented 7 months ago

Grommunio-sync addressed this issue by using microtime() instead of time() to track the start time. Due to the derived code bases it's probably easiest to change this in the inter process data class. https://github.com/Z-Hub/Z-Push/blob/develop/src/lib/core/interprocessdata.php#L116

matidau commented 6 months ago

I've tested it out and it looks good. Have you tested out @grammmichi's comment?

Happy to merge this either way if you want to remove the debug log lines.

Cheers, Mat

liverpoolfc-fan commented 6 months ago

Grommunio-sync addressed this issue by using microtime() instead of time() to track the start time. Due to the derived code bases it's probably easiest to change this in the inter process data class. https://github.com/Z-Hub/Z-Push/blob/develop/src/lib/core/interprocessdata.php#L116

I tried setting start to microtime and it breaks z-push immediately in a few places. I am sure all are fixable but there could be more places that it will affect.

Fatal error: /usr/share/Z-Push-2.7.1/src/lib/utils/utils.php:721 - Uncaught TypeError: strftime(): Argument #2 ($timestamp) must be of type ?int, string given in /usr/share/Z-Push-2.7.1/src/lib/utils/utils.php:721 /usr/share/Z-Push-2.7.1/src/z-push-top.php:252 A non-numeric value encountered (2) /usr/share/Z-Push-2.7.1/src/z-push-top.php:286 A non-numeric value encountered (2) /usr/share/Z-Push-2.7.1/src/lib/core/loopdetection.php:199 A non-numeric value encountered (2)

For now I think we should go with my suggested fix

liverpoolfc-fan commented 6 months ago

I've tested it out and it looks good. Have you tested out @grammmichi's comment?

Happy to merge this either way if you want to remove the debug log lines.

Cheers, Mat

I have removed the DEBUG lines and added a Comment instead.

Do I need to do anything else at this time, or is the same Pull request still good for it?

Vincent

matidau commented 6 months ago

Thanks Vincent, merged 😊