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

Start preparation for Lwt.async demanding unit Lwt.t #370

Merged
merged 2 commits into from
Aug 30, 2019
Merged

Start preparation for Lwt.async demanding unit Lwt.t #370

merged 2 commits into from
Aug 30, 2019

Conversation

cfcs
Copy link

@cfcs cfcs commented Jul 29, 2018

Please see ocsigen/lwt#603

This patch ignores the ('a,'e) result from q.xmit
(which was also unhandled previously, so I don't think I have changed the behavior of this code).

@avsm
Copy link
Member

avsm commented Nov 28, 2018

This lgtm

@hannesm
Copy link
Member

hannesm commented Nov 30, 2018

then you should merge it. I don't understand these intrinsics of the tcp implementation too well, and am a bit worried about ignoring some result value... not sure whom q.xmit should report its failure (though, atm, it doesn't report it either, so the proposed change preserves semantics) :)

@emillon
Copy link

emillon commented Nov 30, 2018

What do you think about adding an explicit type annotation? This will catch the case where we add an argument to the function, or change its return type.

@cfcs
Copy link
Author

cfcs commented Nov 30, 2018

@emillon Good suggestion, I'll update with that!

@cfcs
Copy link
Author

cfcs commented Nov 30, 2018

@avsm Do you remember what this function (xmit) actually does?

@hannesm suggested that we could make it log when we get a Error _, but I can't figure out if this is supposed to fail, and I felt reluctant to add it in case it was supposed to fail 10k times per second in some cases. If it is not supposed to fail often, I think adding a warning message to the log sounds like a very sensible suggestion, especially if it is supposed to never fail and there are bugs to be fixed hiding in here.

But I feel like I have zero insight into how this codebase works, and I really just wanted to pave the way for the new lwt interface with this PR, so I am hoping I can find someone who feels a bit more for this repository to chime in with their expectations to this patch.

@hannesm
Copy link
Member

hannesm commented Feb 3, 2019

@avsm maybe this should be merged for 3.7.0 as well!?

@avsm
Copy link
Member

avsm commented Feb 3, 2019

I still need to review this and refresh my memory about the implications...

@emillon emillon mentioned this pull request Feb 3, 2019
Open
@emillon
Copy link

emillon commented Feb 3, 2019

I think that the conservative option is to fix the API breakage and add the >|= with the type annotation.
I opened #392 to track the question of whether it should be ignored or not - we can replace the TODO by a link to that issue (this was already ignored before).

@cfcs
Copy link
Author

cfcs commented Jun 27, 2019

So, since this patch doesn't change semantics of the current code, should we merge it now?

Then the decision on whether or not we're handling potential xmit failure adequately can be made in #392 ?

ping @avsm

@cfcs
Copy link
Author

cfcs commented Aug 20, 2019

ocsigen/lwt#603 (comment)

@avsm
Copy link
Member

avsm commented Aug 30, 2019

This can indeed be released now; thanks!

@avsm avsm merged commit 451c566 into mirage:master Aug 30, 2019
hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 8, 2020
@hannesm
[new release] tcpip (4.1.0)
89cf541 
CHANGES:

* Revert "Ipv4.Fragments use a Lru.M.t instead of Lru.F.t" (mirage/mirage-tcpip#423 by @hannesm)
  A Lru.M.t allocates a Hashtbl.t of size = capacity (= 256 * 1024 in our case),
  this leads to excessive ~2MB memory consumption for each Fragment cache,
  reported by @xaki23 in mirage/qubes-mirage-firewall#93
* use SOCK_RAW for an ICMP socket in the unix sockets API (previously used
  SOCK_DGRAM which did not work)
  reported by @justinc1 in mirage/mirage-tcpip#358, fixed in mirage/mirage-tcpip#424 by @hannesm
* tcp is now compatible with lwt >= 5.0.0 (where Lwt.async requires a function
  of (unit -> unit Lwt.t) (mirage/mirage-tcpip#370 mirage/mirage-tcpip#425 @cfcs @hannesm, issue mirage/mirage-tcpip#392 @emillon)
* Add a dependency on dune-configurator to support dune 2.0.0 (mirage/mirage-tcpip#421 @avsm)
@hannesm hannesm mentioned this pull request Feb 8, 2020
Merged
mgree pushed a commit to mgree/opam-repository that referenced this pull request Feb 19, 2020
@hannesm @mgree
[new release] tcpip (4.1.0)
8e35151 
CHANGES:

* Revert "Ipv4.Fragments use a Lru.M.t instead of Lru.F.t" (mirage/mirage-tcpip#423 by @hannesm)
  A Lru.M.t allocates a Hashtbl.t of size = capacity (= 256 * 1024 in our case),
  this leads to excessive ~2MB memory consumption for each Fragment cache,
  reported by @xaki23 in mirage/qubes-mirage-firewall#93
* use SOCK_RAW for an ICMP socket in the unix sockets API (previously used
  SOCK_DGRAM which did not work)
  reported by @justinc1 in mirage/mirage-tcpip#358, fixed in mirage/mirage-tcpip#424 by @hannesm
* tcp is now compatible with lwt >= 5.0.0 (where Lwt.async requires a function
  of (unit -> unit Lwt.t) (mirage/mirage-tcpip#370 mirage/mirage-tcpip#425 @cfcs @hannesm, issue mirage/mirage-tcpip#392 @emillon)
* Add a dependency on dune-configurator to support dune 2.0.0 (mirage/mirage-tcpip#421 @avsm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cfcs @avsm @hannesm @emillon