10000 fix(sendfile): Flush the out_channel used in the fallback path by alan-j-hu · Pull Request #8288 · ocaml/dune · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(sendfile): Flush the out_channel used in the fallback path #8288

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
Merged
merged 1 commit into from
Jul 28, 2023
Merged

fix(sendfile): Flush the out_channel used in the fallback path #8288

merged 1 commit into from
Jul 28, 2023

Conversation

alan-j-hu
Copy link
Contributor

Fixes #8284

@alan-j-hu
Copy link
Contributor Author

This code is a little ugly. Because the | exception Unix.Unix_error (EINVAL, "sendfile", _) -> case has separate cleanup rules, I can't use Exn.protect to close the src and dst descriptors. I'm trying to see if there's a cleaner way to write the code.

@alan-j-hu alan-j-hu force-pushed the flush-channel branch 2 times, most recently from af3a18b to 36c9867 Compare July 27, 2023 23:02
@alan-j-hu
Copy link
Contributor Author

Made the code cleaner and made the diff smaller by adding an explicit flush oc instead of having two different cleanup paths.

@alan-j-hu
Copy link
Contributor Author

Added a code comment explaining why the explicit flush is necessary.

@rgrinberg rgrinberg requested a review from emillon July 28, 2023 06:02
@rgrinberg
Copy link
Member

Thanks for the fix. I tweaked a little to always close with the channel api when using channels. I think that's a little safer and makes sure that all of the channel's invariants are respected.

@emillon
Copy link
Collaborator
emillon commented Jul 28, 2023

Nice detective work! The fix is a bit complicated but so is the problem so I'd say that's ok. I'd feel safer with a test so I added one in #8292, to merge before this PR. Thanks!
(did you manage to test that it fixes your other opam installation issues?)

@alan-j-hu @emillon
fix(sendfile): Flush the out_channel used in the fallback path
98e6d76 
Fixes ocaml#8284

Signed-off-by: Alan Hu <alanh@ccs.neu.edu>
@emillon
Copy link
Collaborator
emillon commented Jul 28, 2023

I rebased and top of the fix and it's all good to me.

@rgrinberg rgrinberg merged commit 22c63df into ocaml:main Jul 28, 2023
@alan-j-hu
Copy link
Contributor Author

@rgrinberg Did you see the review comments I left? I'm pretty sure that the exception was forgotten to be reraised.

@rgrinberg
Copy link
Member

Nope. I didn't see them. Maybe you forgot to submit the comments? Will fix though.

@alan-j-hu
Copy link
Contributor Author
alan-j-hu commented Jul 28, 2023 •
edited
Loading

Holy moly, "Pending" is such misleading UI. If not for today, I would not have realized that just commenting on a piece of code in review doesn't submit it.

Now the buggy code was committed to main under my name. :(

@alan-j-hu
Copy link
Contributor Author

I can confirm that my installation issues are fixed when I pinned the dune package to the Git repo.

This was referenced Jul 31, 2023
Closed
Merged
emillon pushed a commit to emillon/dune that referenced this pull request Jul 31, 2023
@alan-j-hu @emillon
fix(sendfile): Flush the out_channel used in the fallback path (ocaml…
f3389cd 
…#8288)

Fixes ocaml#8284

Signed-off-by: Alan Hu <alanh@ccs.neu.edu>
emillon added a commit to emillon/dune that referenced this pull request Jul 31, 2023
@emillon
backport ocaml#8288
c947405 
- test: add a repro for ocaml#8284 (ocaml#8292)
- fix(sendfile): Flush the out_channel used in the fallback path (ocaml#8288)

Signed-off-by: Alan Hu <alanh@ccs.neu.edu>
Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Jul 31, 2023
@emillon
backport #8288 (#8306)
c1a4254 
- test: add a repro for #8284 (#8292)
- fix(sendfile): Flush the out_channel used in the fallback path (#8288)

Signed-off-by: Alan Hu <alanh@ccs.neu.edu>
Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon mentioned this pull request Jul 31, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 31, 2023
@emillon
[new release] dune (15 packages) (3.9.3)
c5f254a 
CHANGES:

- Fix flushing when using `sendfile` fallback (ocaml/dune#8288, @alan-j-hu)
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Aug 10, 2023
@alan-j-hu
fix(sendfile): Flush the out_channel used in the fallback path (ocaml…
4fbe7d1 
…#8288)

Fixes ocaml#8284

Signed-off-by: Alan Hu <alanh@ccs.neu.edu>
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Aug 10, 2023
@alan-j-hu
fix(sendfile): Flush the out_channel used in the fallback path (ocaml…
734fd86 
…#8288)

Fixes ocaml#8284

Signed-off-by: Alan Hu <alanh@ccs.neu.edu>
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@emillon @nberth
[new release] dune (15 packages) (3.9.3)
e181121 
CHANGES:

- Fix flushing when using `sendfile` fallback (ocaml/dune#8288, @alan-j-hu)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emillon emillon Awaiting requested review from emillon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dune upgrades to 3.9.2 but fails to build other packages (Mint, ecryptfs)
3 participants
@alan-j-hu @rgrinberg @emillon
0