Handle I/O on background thread#106
Conversation
This is necessary because a socket connection can be closed while there is still an outstanding callback that references the HttpRequest object. Previously, when that happened, the close() method would get called twice and it would result in the object being deleted twice.
| // deletion of HttpRequest objects happens on the background thread, and so | ||
| // the lifetime of the Environment can't be strictly tied to the lifetime of | ||
| // the HttpRequest. | ||
| boost::shared_ptr<Rcpp::Environment> _env; |
There was a problem hiding this comment.
If you use a shared_ptr here how can you ensure that the environment won't be deleted on the background thread? I'm not sure what the best practice is here but I suspect it might be a naked pointer after all.
There was a problem hiding this comment.
In the implementation, there's a deleter function that ensures that the deletion happens on the main thread:
https://github.com/rstudio/httpuv/blob/3baaf620/src/httprequest.cpp#L146-L154
https://github.com/rstudio/httpuv/blob/3baaf620/src/auto_deleter.h#L13-L36
|
|
||
| void trace(const std::string& msg); | ||
| // TODO: Need a simpler, more robust construct for this | ||
| int _ref_count; |
There was a problem hiding this comment.
Is this obsolete? Doesn't seem to be read anywhere.
|
|
||
| _handling_request = true; | ||
| _headers.clear(); | ||
| _response_scheduled = false; |
There was a problem hiding this comment.
_bytesRead = 0;? Or remove _bytesRead if we don't need it.
I'm currently unsure whether the new ~Socket code is safe, or if there should be a deleter function added to the shared_ptr.
| // kept going and scheduled another call into the main thread to send a | ||
| // response. | ||
| void responseScheduled(); | ||
| bool isResponseScheduled(); |
There was a problem hiding this comment.
Don't really understand this--I'd like to talk about this and whether there are any race conditions (no mutexes)
|
|
||
| // If the Callback class were integrated into later, this wouldn't be | ||
| // necessary -- later could accept a void(Callback*) function. | ||
| void invoke_callback(void* data); |
There was a problem hiding this comment.
Might want to add a convenience function that takes a boost::function<void(void)> and invokes later for you. Lots of the code in httprequest.cpp seems to follow the same pattern and would be easier to read if it was just invoke_later(boost::bind(&method, shared_from_this(), args...)).
wch
force-pushed
the
background-thread
branch
from
508ba1a to
ff24146
Compare
wch
force-pushed
the
background-thread
branch
from
ff24146 to
23a53f9
Compare
|
Things to do before merging:
|
wch
force-pushed
the
background-thread
branch
from
5b28149 to
e676c32
Compare
3 participants
No description provided.