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

fix: no remove peers #16

Closed
wants to merge 2 commits into from
Closed

Conversation

ThaUnknown
Copy link
Contributor

removes peers removal from worker responses as its unreliable

@gfodor
Copy link
Owner

gfodor commented Aug 14, 2022

Can you elaborate on this?

@gfodor
Copy link
Owner

gfodor commented Aug 14, 2022

Oh, noticed it's a draft. Nvmd.

@ThaUnknown
Copy link
Contributor Author

#15

@ThaUnknown ThaUnknown marked this pull request as ready for review August 15, 2022 19:13
@gfodor
Copy link
Owner

gfodor commented Aug 17, 2022

Possible to update this to remove peers that aren't connected, instead of all of them? This is there basically to clean those up i/fwhen they timeout from the signalling side.

@ThaUnknown
Copy link
Contributor Author

ThaUnknown commented Aug 17, 2022

Possible to update this to remove peers that aren't connected, instead of all of them? This is there basically to clean those up i/fwhen they timeout from the signalling side.

that will remove peers who take a long time to connect, which I had happen

those probably should GC when they fail to connect?

idk how to manage either of those things tbh, I haven't looked at the methods of this lib too much

@gfodor
Copy link
Owner

gfodor commented Aug 19, 2022

The server timeout is 2 minutes and the ICE timeout is typically like 30s. If the peers are taking long to connect I’m guessing they never were going to connect, not that they would eventually.

@gfodor
Copy link
Owner

gfodor commented Aug 19, 2022

I’m up for revisiting this cleanup in the way you suggest after we merge the version that maintains active connections. I would need to set up a testing environment and so on to do a full test to make sure that doesn’t regress anything - but the simpler change where we don’t disconnect active ones seems obviously safe so let’s land that first.

@gfodor gfodor closed this Oct 26, 2023
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