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

Duplicate-Pings-With-Same-Timestamp #56

Merged

Conversation

liverpoolfc-fan
Copy link
Contributor

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):

  • OS: Rocky Linux 9
  • PHP Version: 8.1
  • Backend for: zimbra
  • and Version: 2.7.1

Smartphone (please complete the following information):

  • Device: Samsung S10
  • OS: Android 12
  • Mail App GMail
  • Version Android-Mail/2024.01.28...

@matidau
Copy link
Collaborator

matidau commented Feb 16, 2024

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
Copy link

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
Copy link
Collaborator

matidau commented Apr 29, 2024

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
Copy link
Contributor Author

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

Removed DEBUG statements and associated branches.
Added comment
@liverpoolfc-fan
Copy link
Contributor Author

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 matidau merged commit 4a7850b into Z-Hub:develop Apr 29, 2024
2 checks passed
@matidau
Copy link
Collaborator

matidau commented Apr 29, 2024

Thanks Vincent, merged 😊

@liverpoolfc-fan liverpoolfc-fan deleted the Duplicate-Pings-With-Same-Timestamp branch April 30, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants