Conversation
Tpt
left a comment
Tpt
left a comment
There was a problem hiding this comment.
Thank you for this! I think there is a painful case not supported well but I might just be wrong
| } | ||
|
|
||
| impl ConnectionWatch { | ||
| /// Blocks until the client closes the connection or `timeout` elapses. |
There was a problem hiding this comment.
I think there is a painful edge case. If the client never write anything until much after the timeout elapse and no error happens TcpStream::peek will block until then, making the function block until much after timeout. I am not sure we can reliably put a timeout around TcpStream. The cleanest approach is maybe to drop the timeout parameter here and keep having two threads in Oxigraph server. But there is maybe a good solution I am missing. What do you think?
There was a problem hiding this comment.
I hadn't thought about this at all. I reproduced it myself and you were right. With a 3s global timeout and client that never writes anything (wait_closed(Some(Duration::from_millis(100)))) returns after 3.2s and without a global timeout it does not return at all.
I can drop the timeout parameter as suggested and keep the timeout thread in Oxigraph.
…ebug wait_closed no longer takes a timeout parameter. The socket options that a reliable timeout would need are shared with the body reader, so the method now waits at most one global timeout and returns false if the connection is still open. Callers loop with their own stop condition.
| /// The wait period is the [global timeout](Server::with_global_timeout) of the server. | ||
| /// It is 100ms if the client already sent bytes that are not read yet. | ||
| /// Without a global timeout, this method blocks until the client closes the connection or sends bytes. | ||
| pub fn wait_closed(&self) -> bool { |
There was a problem hiding this comment.
This method name is quite confusing imho. The method does not wait for the connection to close but until either the connection is closed or the something failed or there is still data plus 100mns. Imho:
- either we make the function blocking until the connection is effectively closed (matches the function name).
- either we make this function something
wait_for_changethat waits until there is a change in the connection (new data, an error...). But then we will likely need to surface something to the user.
If we want to go the blocking function we need to figure out a proper way to do that taking into account fun things like "we want to detect that the oxhttp server actually dropped its clone of TcpStream to close the connection for the client". So, maybe doing a clone of the TcpStream is not the way to go. If you want I can give it a try.
There was a problem hiding this comment.
If you want I can give it a try.
Please go ahead, you have a better understanding of the different quirks this change needs to take into account
(I am developing an app in which users write SPARQL queries that can take an very long time to complete due to the size of the database and thus canceling an in-flight request would help.)
2 participants
accept_requestclones the stream and inserts a small handle intorequest.extensions_mut()before callingon_requestPrecondition for fixing oxigraph/oxigraph#1874