Skip to content

Improve socket handling #414

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Improve socket handling #414

wants to merge 10 commits into from

Conversation

Gsantomaggio
Copy link
Member

@Gsantomaggio Gsantomaggio commented Apr 15, 2025

  • Improve socket handling by using the native API socket.Connected instead of a local variable to check if the connection is still open or not
  • handle in a better way the close work flow
  • use
  reader = PipeReader.Create(stream, new StreamPipeReaderOptions(leaveOpen: true));
  writer = PipeWriter.Create(stream, new StreamPipeWriterOptions(leaveOpen: true));

To create the reader and writer

  • Add try catch to the message handler in case it fails for some reason it will be logged.
  • Remove one test. There was a bug, and it was not deterministic. ( many other tests cover the same functions )

Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Gabriele Santomaggio <[email protected]>
@Gsantomaggio Gsantomaggio added this to the 1.8.13 milestone Apr 18, 2025
@Gsantomaggio Gsantomaggio marked this pull request as ready for review April 18, 2025 12:39
@Gsantomaggio
Copy link
Member Author

@jonnepmyra, do you have a chance to test it?

@jonnepmyra
Copy link
Contributor

@jonnepmyra, do you have a chance to test it?

Yes, but won't be able to test until Thursday next week 😔

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.

2 participants