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

chore: close px connections after resp #1746

Merged
merged 1 commit into from
May 25, 2023
Merged

chore: close px connections after resp #1746

merged 1 commit into from
May 25, 2023

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented May 19, 2023

  • Since peer exchange protocol is req/resp and after exchanging the req/resp it's not very likely that the stream will be reused, we shall close the stream.
  • Note that .closeWithEOF() is used instead of .close() see.
  • A test is added that will fail without the changes introduced in this PR.
  • Note that the stream is closed but not the connection.

@alrevuelta alrevuelta self-assigned this May 23, 2023
@alrevuelta alrevuelta added this to the Release 0.18.0 milestone May 23, 2023
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! And good catch. Should we do the same for other req/resp protocols?

@alrevuelta
Copy link
Contributor Author

Should we do the same for other req/resp protocols?

@jm-clius I would lightpush for sure and perhaps also store?

But unsure given:

I imagine the expected behaviour would be to close streams for "pure" req/resp protocols after every resp and to keep it open if there's a high likelihood for follow-up requests (e.g. filter push, subsequent store queries, etc.)

@alrevuelta alrevuelta merged commit 3c2d289 into master May 25, 2023
14 checks passed
@alrevuelta alrevuelta deleted the close-conns branch May 25, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants