-
Notifications
You must be signed in to change notification settings - Fork 205
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
Implement v2 client GET functionality #972
Whoa there!
You have triggered an abuse detection mechanism.
Please wait a few minutes before you try again;
in some cases this may take up to an hour.
Changes from 1 commit
4625e96
f07e820
885c131
6848663
a48afb1
225f2a3
d265f6a
e9d91c5
dd3c262
88df865
505a1f0
2b87633
cf1cd80
53893d8
0373dd7
826a026
975b6e5
0666d24
0a49aa5
1193ce7
496e277
2d392ff
db51291
4f3280c
aaa1342
9be51e6
cc6b9a1
ae926c7
f82d128
017a48c
b645370
03f8018
ef3944d
78cab0d
e27d3ea
f6126af
28c3d02
036a222
7b66df6
ad3dc97
6930a47
ec190ca
d27c463
840ca9a
16f0c74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>
Whoa there!
You have triggered an abuse detection mechanism.
Please wait a few minutes before you try again;
in some cases this may take up to an hour.
There are no files selected for viewing
Whoa there!
You have triggered an abuse detection mechanism.
Please wait a few minutes before you try again;
in some cases this may take up to an hour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it not goroutine safe? Pretty sure this is used in the proxy which can receive parallel requests that are treated in separate goroutines. Do we have a race condition right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This client wraps a
RelayClient
, which doesn't specify thread safety. So I started with "not threadsafe", for the sake of cautionBut more fundamentally, my question would be if the proxy actually needs parallelism here. The requests are treated in separate goroutines, but afaict they are being served by a single
Manager
, which could synchronize access to this client.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to synchronize access to the client in proxy though! Client requests can take a long time to deal with because of multiple network routines. Plus when dispersing you need to wait for the CERTIFIED status to be ready which can take up to 10 seconds.
We should be much more fine-grained with synchronization. I would suggest we put a mutex around rand (would say that we don't even care if theres a race on this source of randomness, but perhaps it can actually cause some weird stuff to happen...?).
Also seems like accountant in DisperserClient is thread safe (see this discussion) so I think we should be fine for concurrent support with these 2, but let's do a quick check to make sure there's nothing else we're missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed synchronously, there doesn't appear to be anything that would make
RelayClient
unsafe.We also discussed that synchronizing in the proxy is not acceptable, due to very long times for dispersal (~10 seconds)
I've updated this comment accordingly e27d3eaa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait but its not! Missing mutex around rand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capturing our synchronous discussion
I added an additional comment about this 16f0c748