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: avoid finishing write futures while stream is being closed #1032

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

diegomrsantos
Copy link
Contributor

No description provided.

@diegomrsantos diegomrsantos force-pushed the mplex-futures branch 2 times, most recently from c0b6bab to 21acc24 Compare February 27, 2024 15:52
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.77%. Comparing base (e9b4561) to head (9c1d879).
Report is 1 commits behind head on unstable.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable    #1032      +/-   ##
============================================
+ Coverage     82.75%   82.77%   +0.02%     
============================================
  Files            91       91              
  Lines         15616    15620       +4     
============================================
+ Hits          12923    12930       +7     
+ Misses         2693     2690       -3     
Files Coverage Δ
libp2p/muxers/mplex/lpchannel.nim 83.87% <100.00%> (+2.00%) ⬆️

@diegomrsantos diegomrsantos force-pushed the mplex-futures branch 2 times, most recently from 2a24d14 to fde07e9 Compare February 28, 2024 12:49
@@ -117,10 +118,10 @@ proc reset*(s: LPChannel) {.async.} =
await s.conn.close()
trace "Can't send reset message", s, conn = s.conn, msg = exc.msg

asyncSpawn resetMessage()
await resetMessage()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone know why it was asyncSpawn here?

Copy link
Contributor

Choose a reason for hiding this comment

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

the reset message is advisory - basically, it's a best-effort message that is sent in the hopes it will arrive and should therefore (in theory) not be allowed to block resetting the stream.

There's a bit of a terminology gap in libp2p as well - closing and resetting are used somewhat interchangeably.

If we take close to mean what it does in BSD sockets, it's an operation that cannot fail / always succeeds - it can return an error but even if it does, the socket is considered closed afterwards.

In libp2p, close means more or less what shutdown means in bsd and reset is the "irrevokable close now" function - it's important that such a function doesn't get "stuck".

I'm not sure what the consequences of waiting here would be tbh, it deserves some thought and lots of testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please point out where it says it's a best-effort message? It doesn't seem like that here https://github.com/libp2p/specs/blob/master/mplex/README.md#resetting-a-stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should generally only be used on error;

the nature of the message is such that it is used when the stream is in an inconsistent state, ie when there is an error - because the stream is inconsistent, you also cannot guarantee that the message will be received by the other end and therefore you cannot rely on there being an answer to the message (it can get lost either locally, on the way to the other peer or the other peer may decide to not reply and if the other client is not reading the stream, it may get "blocked" forever).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you cannot rely on there being an answer to the message

I don't think we expect any answer, do we?

Copy link
Contributor

Choose a reason for hiding this comment

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

from the point of view of await, finishing the future is an answer - we don't expect an explicit message as answer, but from a buffer perspective, the future will not be finished until it has been given to the OS and the OS might not accept it until the other side sends an ACK confirming the receipt of earlier queued-up bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I reverted the change. I think it's unrelated to the PR anyway.

@diegomrsantos diegomrsantos changed the title Mplex futures fix: avoid finished write futures while stream is being closed Feb 28, 2024
@diegomrsantos diegomrsantos changed the title fix: avoid finished write futures while stream is being closed fix: avoid finished] write futures while stream is being closed Feb 28, 2024
@diegomrsantos diegomrsantos changed the title fix: avoid finished] write futures while stream is being closed fix: avoid finishing write futures while stream is being closed Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: backlog
Development

Successfully merging this pull request may close these issues.

2 participants