status-go
status-go copied to clipboard
status-im
Server Port Race Condition Fix
What's Changed?
I've introduced portManager which is responsible for maintaining segregated access to the port field via the use of rwLock sync.RWMutex and mustRead sync.Mutex
rwLockestablishes a standard read write mutex that allows consecutive reads and exclusive writesmustReadforcesMustGetPort()to wait until port has a none 0 value
mustRead guards MustGetPort and only allows access to the underlying port field once the unlocked. This causes an dependent function to wait until the port number is available.
Why Change?
Before this PR we had a number of issues:
- Calling
PairingServer.MakeConnectionParams()has a race condition that intermittently returns an error because theServerport is not yet set - Calling
Server.MakeBaseURL()and all dependent functions have a race condition that intermittently returns an invalid URL because the port has not yet been set. afterPortChangedfunc(port int)used to be only called once in the server's whole life cycle, meaning updated port numbers would not trigger.
NOTE FOR CODE REVIEWERS
This PR has heavy use of mutex and although I've took great care to implement this fix / feature as sensibly as I saw fit, there may very well be things I've overlooked. Don't be fooled by the simplicity of the PR.
NOTE FOR QA
This PR has the potential to cause unexpected hanging on getting
- audio
- identicons
- images
- ipfs
- account Images (profile picture)
- contact Images (contact profile pictures)
Pull Request Checklist
- [ ] Have you updated the documentation, if impacted (e.g. docs.status.im)?
- [ ] Have you tested changes with mobile?
- [ ] Have you tested changes with desktop?
![status-github-bot[bot] avatar](https://avatars.githubusercontent.com/in/8157?v=4 "Published by a pub.dev verified publisher"){kind=small}
Jenkins Builds
Click to see older builds (49)
| :grey_question: | Commit | :hash: | Finished (UTC) | Duration | Platform | Result |
|---|---|---|---|---|---|---|
| :heavy_check_mark: | a5f793cd | #1 | 2022-10-12 12:55:35 | ~3 min | linux |
:package:zip |
| :heavy_check_mark: | a5f793cd | #1 | 2022-10-12 12:58:55 | ~6 min | ios |
:package:zip |
| :heavy_check_mark: | a5f793cd | #1 | 2022-10-12 12:58:59 | ~6 min | android |
:package:aar |
| :heavy_check_mark: | 7e28151d | #2 | 2022-10-12 14:12:53 | ~3 min | linux |
:package:zip |
| :heavy_check_mark: | 7e28151d | #2 | 2022-10-12 14:12:58 | ~3 min | ios |
:package:zip |
| :heavy_check_mark: | 7e28151d | #2 | 2022-10-12 14:15:07 | ~5 min | android |
:package:aar |
| :heavy_check_mark: | 759a2036 | #3 | 2022-10-12 14:44:14 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | 759a2036 | #3 | 2022-10-12 14:44:59 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | 759a2036 | #3 | 2022-10-12 14:46:38 | ~4 min | ios |
:package:zip |
| :heavy_check_mark: | 9c0edefd | #4 | 2022-10-12 14:46:18 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | 9c0edefd | #4 | 2022-10-12 14:48:37 | ~1 min | ios |
:package:zip |
| :heavy_check_mark: | 9c0edefd | #4 | 2022-10-12 14:50:35 | ~5 min | android |
:package:aar |
| :heavy_check_mark: | 03d4f0e3 | #5 | 2022-10-12 14:53:26 | ~1 min | linux |
:package:zip |
| :heavy_check_mark: | 03d4f0e3 | #5 | 2022-10-12 14:53:38 | ~2 min | ios |
:package:zip |
| :heavy_check_mark: | 03d4f0e3 | #5 | 2022-10-12 14:54:41 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | 5c699a15 | #6 | 2022-10-12 15:24:20 | ~2 min | ios |
:package:zip |
| :heavy_check_mark: | 5c699a15 | #6 | 2022-10-12 15:24:29 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | 5c699a15 | #6 | 2022-10-12 15:26:39 | ~5 min | android |
:package:aar |
| :heavy_check_mark: | eb96ea9c | #7 | 2022-10-14 15:48:45 | ~1 min | linux |
:package:zip |
| :heavy_check_mark: | eb96ea9c | #7 | 2022-10-14 15:49:05 | ~2 min | ios |
:package:zip |
| :heavy_check_mark: | eb96ea9c | #7 | 2022-10-14 15:50:36 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | e435e00c | #8 | 2022-10-17 09:28:45 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | e435e00c | #8 | 2022-10-17 09:29:44 | ~3 min | ios |
:package:zip |
| :heavy_check_mark: | e435e00c | #8 | 2022-10-17 09:30:24 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | f0477d83 | #9 | 2022-10-17 11:45:00 | ~1 min | linux |
:package:zip |
| :heavy_check_mark: | f0477d83 | #9 | 2022-10-17 11:45:47 | ~2 min | ios |
:package:zip |
| :heavy_check_mark: | f0477d83 | #9 | 2022-10-17 11:46:49 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | 542529d3 | #10 | 2022-10-17 11:51:51 | ~2 min | ios |
:package:zip |
| :heavy_check_mark: | 542529d3 | #10 | 2022-10-17 11:53:21 | ~4 min | linux |
:package:zip |
| :heavy_check_mark: | 542529d3 | #10 | 2022-10-17 11:54:57 | ~5 min | android |
:package:aar |
| :heavy_check_mark: | 39384ced | #11 | 2022-10-19 10:04:41 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | 39384ced | #11 | 2022-10-19 10:05:13 | ~3 min | ios |
:package:zip |
| :heavy_check_mark: | 39384ced | #11 | 2022-10-19 10:06:20 | ~4 min | android |
:package:aar |
| :heavy_check_mark: | b78d7894 | #12 | 2022-10-21 01:05:34 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | b78d7894 | #12 | 2022-10-21 06:57:36 | ~4 min | android |
:package:aar |
| :x: | b78d7894 | #12 | 2022-10-21 10:22:23 | ~1 min | ios |
:page_facing_up:log |
| :x: | 9b4fee51 | #13 | 2022-10-21 12:18:22 | ~2 min | ios |
:page_facing_up:log |
| :heavy_check_mark: | 9b4fee51 | #13 | 2022-10-21 12:18:26 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | 9b4fee51 | #13 | 2022-10-21 12:19:41 | ~3 min | android |
:package:aar |
| :x: | e8b11fde | #14 | 2022-10-21 12:19:17 | ~52 sec | ios |
:page_facing_up:log |
| :heavy_check_mark: | e8b11fde | #14 | 2022-10-21 12:20:33 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | e8b11fde | #14 | 2022-10-21 12:22:58 | ~3 min | android |
:package:aar |
| :x: | 9a997d02 | #15 | 2022-10-21 15:21:15 | ~58 sec | ios |
:page_facing_up:log |
| :heavy_check_mark: | 9a997d02 | #15 | 2022-10-21 15:23:22 | ~3 min | linux |
:package:zip |
| :heavy_check_mark: | 9a997d02 | #15 | 2022-10-21 15:23:52 | ~3 min | android |
:package:aar |
| :x: | 51a3e2f6 | #16 | 2022-10-21 15:39:29 | ~1 min | ios |
:page_facing_up:log |
| :heavy_check_mark: | 51a3e2f6 | #16 | 2022-10-21 15:40:29 | ~2 min | linux |
:package:zip |
| :heavy_check_mark: | 51a3e2f6 | #16 | 2022-10-21 15:43:20 | ~4 min | android |
:package:aar |
| :x: | 51a3e2f6 | #17 | 2022-10-21 16:01:44 | ~3 min | ios |
:page_facing_up:log |
| :grey_question: | Commit | :hash: | Finished (UTC) | Duration | Platform | Result |
|---|---|---|---|---|---|---|
| :heavy_check_mark: | 9e7aeb2e | #17 | 2022-10-21 21:51:49 | ~2 min | linux |
:package:zip |
| :x: | 9e7aeb2e | #18 | 2022-10-21 21:51:52 | ~2 min | ios |
:page_facing_up:log |
| :heavy_check_mark: | 9e7aeb2e | #17 | 2022-10-21 21:53:25 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | b8b7d386 | #18 | 2022-10-25 11:57:48 | ~1 min | linux |
:package:zip |
| :heavy_check_mark: | b8b7d386 | #18 | 2022-10-25 11:59:30 | ~3 min | android |
:package:aar |
| :heavy_check_mark: | b8b7d386 | #19 | 2022-10-25 12:00:06 | ~4 min | ios |
:package:zip |
Hi @Samyoul, thank you for the PR !
I've performed regression testing on mobile here https://github.com/status-im/status-mobile/pull/14167. Please, take a look at the following issues:
ISSUE 1 Share/save image buttons stop responding after image was once shared/saved
Steps:
- Send some image in any kind of chat
- Open the sent image
- Tap save or share button
- Try to save or share image (you can try this action on any other image in chat) once again
- Observe the result
Actual result: tapping on save button results in closing image but the image is not saved. tapping on share image doesn not result in opening bottom sheet with sending options
ISSUE 2 Endless loader after opening image in chat
Steps:
- Send 2 or more images in any kind of chat
- Open any of sent image
- Tap save or share button
- Close the image
- Try to open other images
- Observe the result
Actual result: Endless loader, image is not opened
Both issues are reproducible on Android. On IOS it is not reproducible every time, though I also faced the described behaviour on IOS too.
Video and logs for both ISSUE 1 and ISSUE 2 are attached below
https://user-images.githubusercontent.com/97245802/196207591-041fa595-43a9-470c-96c9-f4a2d76bb0bb.mp4
A little update:
So what seems to be happening, I think, is that sharing or saving puts the application temporally into the background, which triggers the MediaServer to restart, which may or may not regenerate a new port number which may or may not be updated in the UI. I've moved when a signal is triggered but it triggers only when the port number updates, this is expected behaviour, but the UI may or may not be responding to this.
Another potential line of inquiry is that he UI may have a really aggressive timeout on localhost requests.