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

Forbid nesting calls to Lwt_main.run #609

Merged
merged 6 commits into from Dec 15, 2019
Merged

Forbid nesting calls to Lwt_main.run #609

merged 6 commits into from Dec 15, 2019

Conversation

aantron
Copy link
Collaborator

@aantron aantron commented Aug 23, 2018

cc @fare

Resolves #607.

let backtrace =
try raise Exit
with Exit -> Printexc.get_backtrace ()
in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to try recording stack traces of where Lwt_main.run was called, rather than add an optional argument.

@Drup
Copy link
Member

Drup commented Aug 23, 2018

I don't really object to it, but have you checked on opam that this doesn't break anything ?

@Drup
Copy link
Member

Drup commented Aug 23, 2018

Actually, opam will probably not tell you that anyway .... Anyway, I think it's a breaking change in general, even if it has been strongly verboten for years.

@aantron
Copy link
Collaborator Author

aantron commented Aug 23, 2018

@fare If at all easy to do, could you check that this actually catches the error in the specific setup you had? I don't know enough about dynamic loading in OCaml and/or ocsigenserver to know whether the new internal run_already_called flag will be shared or not.

@aantron
Copy link
Collaborator Author

aantron commented Aug 23, 2018

@Drup I just searched the revdeps for it, the number of occurrences seems too large to analyze by hand. Many are in tests and examples, but there are plenty in the main source code of packages. Let's see if this helps catch the problem @fare was having. If so, we'll tag it breaking and put it in in 5.0.0 with all the proper announcements, etc.

@fare
Copy link

fare commented Aug 24, 2018

Thanks a lot for this patch!

Importing a new lwt requires some setup, because I need to pin not just lwt but several other packages that depend on it. I'll try tomorrow when I have more time.

@fare
Copy link

fare commented Aug 26, 2018

Interestingly enough, the bug disappears when I upgrade lwt... which however did require me to also upgrade ocsigenserver, so this may explain that. Or not.

It's weird that given the same lwt and ocsigenserver, my code should work (or at least look like it works) whether or not I call Lwt_main.run...

@aantron aantron added this to the 5.0.0 milestone Aug 26, 2018
@raphael-proust
Copy link
Collaborator

I don't think it's that weird: if you don't call it, then it's probably called by ocsigenserver after initialisation and so it works; if you do call it, then it probably follows the sequential execution described in #607: “If you were calling Lwt_main.run under a top-level let _ =, it seems to me the other Lwt_main.run must have already returned, so the two calls shouldn't be interfering with each other.”

@aantron
Copy link
Collaborator Author

aantron commented Aug 27, 2018

@fare Can you say which versions of ocsigenserver and Lwt you were using before, and which you are using now?

@fare
Copy link

fare commented Aug 27, 2018

Before: I was using ocsigenserver 2.9 and lwt 3.3.0 from OPAM.

After: I am using lwt from source (your branch) and still ocsigenserver 2.9 it seems. At some point I was also running from source because it was complaining about lwt being too recent, but it looks like this doesn't happen with your branch of lwt.

My understanding is that ocsigenserver was loading my code from within the lwt loop, but then again, I don't really know.

@aantron
Copy link
Collaborator Author

aantron commented Aug 27, 2018

@fare Can you try with Lwt master? But I suspect it will work fine, because if you had a nested Lwt_main.run call with this branch, you would be seeing an exception.

If that's the case, I am not sure that there is a bug to fix or that we have diagnosed the issue correctly.

@fare
Copy link

fare commented Aug 28, 2018

Actually, I don't understand OPAM, and I was probably using the wrong lwt and ocsigenserver. Now I made sure I was using the right one (pinning lwt, cohttp, ocsigenserver, eliom), and somehow, I can't reproduce the issue in this setting, and I don't get an error message either.

@aantron aantron force-pushed the nested-lwt-main-run branch 2 times, most recently from b2ea9d3 to 835ed46 Compare December 15, 2019 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lwt_main.run should fail with prejudice if already running
4 participants