Improvements to how timeouts are handled. #1043
Conversation
|
This pull request introduces 3 alerts when merging ebf9519 into 391b3fa - view on LGTM.com new alerts:
|
| warnings.warn(message) | ||
| self.start(width=self.width, height=self.height, x_display=self.x_display) | ||
| self.reset() | ||
| raise RestartError(message) | ||
| except Exception as e: | ||
| self.server.stop() | ||
| raise (TimeoutError if isinstance(e, TimeoutError) else RuntimeError)( |
There was a problem hiding this comment.
Would it be more clear if we had an except branch for TimeoutError?
There was a problem hiding this comment.
I agree regarding clarity but I'd prefer to have a single branch here as, otherwise, we would be duplicating the same self.server.stop() etc code twice.
ai2thor/controller.py
Outdated
| self.last_event = self.server.receive( | ||
| timeout=max( | ||
| 60.0 * 5, | ||
| self.server_timeout * 5 if self.server_timeout is not None else 0.0 |
There was a problem hiding this comment.
Why is this multiplied by 5 wouldn't it be better for the value to be the actual timeout_value so there aren't unexpected behaviors?
Also seems like the if condition will never be true because there is an assert for self.server_timeout is None in the constructor
There was a problem hiding this comment.
Decided to just create a server_start_timeout parameter. I was trying to keep the number of initialization parameters low but this was probably unnecessary.
Also seems like the if condition will never be true because there is an assert for self.server_timeout is None in the constructor
The assert you're talking about checks that self.server_timeout is None or that it's positive.
| start_t = time.time() | ||
| message = b"" | ||
|
|
||
| while message_size > 0: |
There was a problem hiding this comment.
Is this way of reading with the while loop and directly from the file descriptor any slower? I don't think it should, but an option could be if server_timeout is None then we could use the old function.
There was a problem hiding this comment.
That's already there if look at lines 116-117.
ai2thor/wsgi_server.py
Outdated
| max_attempts = ( | ||
| float("inf") | ||
| if timeout is None or timeout == float("inf") else | ||
| max(int(math.ceil(timeout / 0.5)), 1) |
There was a problem hiding this comment.
A line is a bit hard to read, any reason for / 0.5 vs * 2, making it floating point?
There was a problem hiding this comment.
I've made this a bit clearer, the idea is that the 0.5 references the 0.5 used by the que.get(...) call a few lines layer.
| @@ -1987,6 +1987,16 @@ abstract public class BaseFPSAgentController { | |||
| actionFinished(true); | |||
| } | |||
|
|
|||
| protected IEnumerator waitForSecondsRealtime(int seconds) { | |||
There was a problem hiding this comment.
Don't see this being referenced anywhere
There was a problem hiding this comment.
It's being used by the new Sleep action which is used by the new tests that check that make sure the timeout code works properly.
|
A couple of comments this is great! LGTM after that |
We have code that times out the AI2-THOR unity process if it doesn't respond to a request in some number of seconds when using the WSGI server. Similar code did not seem to exist for the, now default, fifo server. This PR adds this timeout functionality (with tests) and adds a few small typing improvements.