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

Clarify the semantics of Flow.write when the buffer is full #4

Closed
samoht opened this issue Feb 26, 2015 · 5 comments
Closed

Clarify the semantics of Flow.write when the buffer is full #4

samoht opened this issue Feb 26, 2015 · 5 comments

Comments

@samoht
Copy link
Member

samoht commented Feb 26, 2015

In Fflow.write will fill the buffer and return eof if the buffer is full. Lwt_io_flow.write will fill the buffer and then block. One of them is wrong and need to be fixed.

@hannesm
Copy link
Member

hannesm commented Jun 4, 2017

Sorry to hijack this issue, but I stumbled upon a similar issue: mirage-console is a FLOW, and it would be great to have its error behaviour well documented... what should happen if Mirage_console.write can only do a partial write (which may be the case if the underlying socket is a unix domain socket)?

The implementations differ:

  • unix uses Lwt_cstruct.complete (and thus loops till all the bytes are written)
  • xen loops manually until all the bytes are written
  • solo5 uses solo5_console_write via mirage-solo5 from solo5, but discards the returned int (which is the number of bytes written)

The majority (of console implementations) is in favor of blocking until all bytes are written. It would IMHO be very sensible to have a common behaviour amongst all implementations of write -- is there anything depending on @samoht mentioned "returns eof" anyone is aware of?

@hannesm
Copy link
Member

hannesm commented Jun 4, 2017

//cc @mato

ups, looking deeper into solo5, it turns out that console_write is using the hypercall puts, which is implemented to return void (and int platform_puts () returns the string length):

    int rc = write(1, UKVM_CHECKED_GPA_P(hv, p->data, p->len), p->len);
    assert(rc >= 0);

@hannesm
Copy link
Member

hannesm commented Jun 4, 2017

I have a fix proposal for the issue I mentioned above, spanning over 3 PRs: Solo5/solo5#196 mirage/mirage-solo5#19 mirage/mirage-console-solo5#12

@hannesm
Copy link
Member

hannesm commented Dec 17, 2023

This is pretty old issue, but for network streams my opinion is that write should block when the buffer is full. Otherwise you've some task that isn't aware of what to do and when to continue. If the underlying transport is closed, a `Closed error should be returned.

Indeed, we should clarify this in the documentation.

@hannesm
Copy link
Member

hannesm commented Feb 8, 2024

adressed in 7a4eb61 - the call to write blocks until the buffer has been accepted by the implementation. no partial writes.

@hannesm hannesm closed this as completed Feb 8, 2024
hannesm added a commit to hannesm/opam-repository that referenced this issue Feb 8, 2024
CHANGES:

- move Mirage_flow.stats and pp_stats to Mirage_flow_combinators (mirage/mirage-flow#51 @hannesm)
- improve documentation of expected semantics (when write promise is resolved,
  what is done to the underlying flow - addresses mirage/mirage-flow#4 @samoht),
  (mirage/mirage-flow#51 @reynir @dinosaure @hannesm)
- add < coercion to shutdown:
  ``shutdown : flow -> [< `read | `write | `read_write ] -> unit Lwt.t``
  (requested mirage/mirage-flow#50 @reynir, mirage/mirage-flow#52 @hannesm)
hannesm added a commit to hannesm/opam-repository that referenced this issue Feb 8, 2024
CHANGES:

- move Mirage_flow.stats and pp_stats to Mirage_flow_combinators (mirage/mirage-flow#51 @hannesm)
- improve documentation of expected semantics (when write promise is resolved,
  what is done to the underlying flow - addresses mirage/mirage-flow#4 @samoht),
  (mirage/mirage-flow#51 @reynir @dinosaure @hannesm)
- add < coercion to shutdown:
  ``shutdown : flow -> [< `read | `write | `read_write ] -> unit Lwt.t``
  (requested mirage/mirage-flow#50 @reynir, mirage/mirage-flow#52 @hannesm)
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

No branches or pull requests

2 participants