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.

Sign up for GitHub

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

Jump to bottom

Fix C# connection to correctly mark message as sent #3072

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Fix C# connection to correctly mark message as sent #3072

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions csharp/src/Ice/ConnectionI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,15 +607,23 @@ public override bool startAsync(int operation, Ice.Internal.AsyncCallback comple
observerStartWrite(_writeStream.getBuffer());
}

bool completed;
if (_transceiver.startWrite(_writeStream.getBuffer(), completedCallback, this, out completed))
bool messageWritten = false;
bool completedSynchronously =
_transceiver.startWrite(
_writeStream.getBuffer(),
completedCallback,
this,
out messageWritten);
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why the C# startWrite has such a complicated signature compared to the C++ startWrite. In C++, we don't have this "completedSynchronously" return value.

In any case, assuming this complicated C# API is warranted, we should:

  • rename the last parameter of Transceiver.startWrite. Having return value = completedSynchronously and out parameter = completed is too confusing.
  • consider returning a tuple (does this work with the old C# version we use on 3.7?)
  • name the "messageWritten" parameter => messageFullyWritten
  • add a real doc-comment on Transceiver.startWrite

Copy link
Member Author

@pepone pepone Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In any case, assuming this complicated C# API is warranted, we should:

Can we do that in a separate PR?

consider returning a tuple (does this work with the old C# version we use on 3.7?)

It does not work with .NET 4.5

Copy link
Member

@bernardnormier bernardnormier Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this can wait until a follow-up PR.

// If the startWrite call consumed the complete buffer, we assume the message is sent now for
// at-most-once semantics.
if (messageWritten && _sendStreams.Count > 0)
Copy link
Member

Choose a reason for hiding this comment

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

(Not from this PR)
Do you know when we can reach this point with _sendStreams.Count == 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems StartWrite is some times called with a 0 buffer size just to invoke the completion callback. It is not clear why we do it that way, and cannot call it from the finishAsync.

{
// If the write completed immediately and the buffer
if (completed && _sendStreams.Count > 0)
{
// The whole message is written, assume it's sent now for at-most-once semantics.
_sendStreams.First.Value.isSent = true;
}
_sendStreams.First.Value.isSent = true;
}

if (completedSynchronously)
{
// If the write completed synchronously, we need to call the completedCallback.
completedCallback(this);
}
}
Expand Down