Skip to content

Complete Win32 non-blocking implementation#58

Closed
hcorrada wants to merge 4 commits into
rstudio:masterfrom
epiviz:master
Closed

Complete Win32 non-blocking implementation#58
hcorrada wants to merge 4 commits into
rstudio:masterfrom
epiviz:master

Conversation

@hcorrada

Copy link
Copy Markdown
Contributor

Using ideas from Simon Urbanek's 'background' draft package that implements the Rhttpd strategy. Implementation description in src/httpuv.cpp

jcheng5 and others added 2 commits January 30, 2016 14:28
Fully implement solution for running the libuv default loop without blocking in win32.
It is based on Simon Urbanek's 'background' draft package that implements a version
of the Rhttpd strategy. Implementation details are in src/httpuv.cpp file.
Comment thread src/httpuv.cpp Outdated
TerminateThread(server_thread, 0);
server_thread = 0;
if (GetExitCodeThread(thread, &ts) && ts == STILL_ACTIVE)
TerminateThread(thread, 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems really bad--it's almost never a good idea to forcibly terminate a thread from the outside, with the possible exception of when the entire process is about to shut down. It can leave all sorts of state corrupted. The usual approach is to set a flag that the other thread will check, and if it's important for this thread to not continue until it knows the other thread is dead, to wait/join the other thread (in Windows it looks like WaitForSingleObject will work).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll take a look, these lines are straight from 'background' and Rhttpd, so I'll pass along whatever we end up using here to them.

@jcheng5

jcheng5 commented Feb 11, 2016

Copy link
Copy Markdown
Member

Thanks for all the hard work. I know this stuff takes a lot of effort.

I don't fully understand the comment in httpuv.cpp; is the libuv loop run on the main thread, or the background thread?

@jcheng5

jcheng5 commented Feb 11, 2016

Copy link
Copy Markdown
Member

I may also be spending more time on this in the future. It turns out that even in the non-daemonized case, having the libuv loop running on the main thread really hurts I/O performance a lot when R is also busy--it's enough of a problem that some geospatial related features in Leaflet are not worth shipping until we resolve it.

@hcorrada

Copy link
Copy Markdown
Contributor Author

The libuv ends up running in the main thread, through this message passing mechanism. The background thread is only running the 'polling' loop. Again, straight from 'background' and Rhttpd.

What do you have in mind? With the existing 'InputHandler' and this win32 message passing mechanisms, the libuv loop still runs in the main R thread. Perhaps use libuv's thread mechanism directly?

Ok, thanks, I'll add a better solution to thread stopping in the next day or so.

@jjallaire

Copy link
Copy Markdown
Member

If the background thread is running a message loop then you should just be
able to "PostMessage" (async message pass) a custom WM_ message telling it
to terminate (you just then handle that message within the thread by call
ExitThread.

On Fri, Feb 12, 2016 at 8:12 AM, hcorrada notifications@github.com wrote:

The libuv ends up running in the main thread, through this message passing
mechanism. The background thread is only running the 'polling' loop. Again,
straight from 'background' and Rhttpd.

What do you have in mind? With the existing 'InputHandler' and this win32
message passing mechanisms, the libuv loop still runs in the main R thread.
Perhaps use libuv's thread mechanism directly?

Ok, thanks, I'll add a better solution to thread stopping in the next day
or so.


Reply to this email directly or view it on GitHub
#58 (comment).

@hcorrada

Copy link
Copy Markdown
Contributor Author

@jcheng5 Check this thread exit strategy. Let me know if it can be improved. THanks!

Comment thread src/httpuv.cpp Outdated
int ntimes = 10;
int i = 0;

while (i < ntimes && GetExitCodeThread(thread, &ts) && ts == STILL_ACTIVE) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you just want WaitForSingleObject(hThread, millis) here.

@hcorrada

Copy link
Copy Markdown
Contributor Author

With WaitForSingleObject I can take that out. I'll submit another commit soon.

@hcorrada

Copy link
Copy Markdown
Contributor Author

Hey @jcheng5, thanks for the pointer!

@hcorrada

Copy link
Copy Markdown
Contributor Author

Hi, @jcheng5, can this be pulled in?

@paul-shannon

paul-shannon commented Dec 20, 2016

Copy link
Copy Markdown

Hi Hector,

@hcorrada - I cannot tell if your good work, getting Windows httpuv async communications, R <-> browser, made it into release. Could you let me know?

@hcorrada

Copy link
Copy Markdown
Contributor Author

Don't think so... @jcheng5 maybe take a look after rstudio conference?

@paul-shannon

paul-shannon commented Jan 13, 2017 via email

Copy link
Copy Markdown

@jcheng5

jcheng5 commented Apr 6, 2017

Copy link
Copy Markdown
Member

Sorry for the long, long delay on this. I just felt really uncomfortable with the Windows implementation using threads and still do. I believe I may have found a much cleaner solution using SetTimer though, this doesn't need a background thread at all:
https://github.com/jcheng5/later/blob/master/src/later_win32.cpp

I also need to dive deeply back into httpuv to support asynchronous I/O. I'll try to prioritize Win32 support for daemonized httpuv as well, and getting it to a place where I'm comfortable calling it non-experimental.

@paul-shannon

paul-shannon commented Apr 6, 2017 via email

Copy link
Copy Markdown

@hcorrada

hcorrada commented Apr 6, 2017

Copy link
Copy Markdown
Contributor Author

Thanks Joe! That would be awesome!

@wch

wch commented Apr 13, 2018

Copy link
Copy Markdown
Collaborator

I think this is no longer necessary because of #106.

@wch wch closed this Apr 13, 2018
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

Successfully merging this pull request may close these issues.

5 participants