adding await ystore start method in prepare method #299
Conversation
|
|
jzhang20133
force-pushed
the
2.x-new
branch
4 times, most recently
from
36f3f3c
to
1f367f5
Compare
|
Test failures appear to be related to: jupyter-server/jupyter_server_fileid#75 |
jupyter_collaboration/handlers.py
Outdated
| @@ -111,6 +111,7 @@ def exception_logger(exception: Exception, log: Logger) -> bool: | |||
| file = self._file_loaders[file_id] | |||
| updates_file_path = f".{file_type}:{file_id}.y" | |||
| ystore = self._ystore_class(path=updates_file_path, log=self.log) | |||
| await ystore.start() | |||
There was a problem hiding this comment.
In the general case, starting a service can be blocking. Actually, after jupyter-server/pycrdt-websocket#42 starting a YStore will be.
| await ystore.start() | |
| self.create_task(ystore.start()) | |
| await ystore.started.wait() |
There was a problem hiding this comment.
Actually, starting the YStore should not be done here but in the DocumentRoom, since it is optional.
Also, you might want to stop the YStore when stopping the YRoom.
There was a problem hiding this comment.
I think adding await in the DocumentRoom is a good idea and it also help prevent starting the room at two places which could cause race condition as well. I will make that change.
| @@ -100,6 +100,7 @@ async def initialize(self) -> None: | |||
| # try to apply Y updates from the YStore for this document | |||
| read_from_source = True | |||
| if self.ystore is not None: | |||
| await self.ystore.started.wait() | |||
There was a problem hiding this comment.
@jzhang20133 You didn't apply my suggestion.
There was a problem hiding this comment.
@davidbrochart Following your suggestion, we moved the logic here in document room. By not applying your suggestion, are you referring to this lineself.create_task(ystore.start())? I did not add ystore.start logic here because ystore.start() is also called in yroom.broadcast_update() method. And if ystore has been started and then it is started again, an exception could be thrown and the exception could kill task groups. Instead of triggering starting of ystore at multiple places and exception handling at multiple places, we choose to simply wait for the ystore to be started, which is a great idea that you have suggested.
There was a problem hiding this comment.
Thanks, @jzhang20133. I'll let David respond to this when he's back in office next week and we can create a follow-up PR if needed.
@davidbrochart's point below stands. If we do something different from his suggestions, we should include an explanation and wait for his response before merging.
This one is on me folks. I missed this difference in the change before approving the PR. Let's learn from this and make sure we're properly patient going forward.
There was a problem hiding this comment.
@Zsailer @davidbrochart I will make sure I add explanation if change is made differently from suggested and wait for @davidbrochart 's inputs and approval on the difference in the future PRs. Thank you both for your patience and help on reviewing PRs! Really appreciated!!!
There was a problem hiding this comment.
@davidbrochart For this specific change, please feel free to let us know your thought here about not adding this line self.create_task(ystore.start()). I can address accordingly in another PR.
There was a problem hiding this comment.
yes, it just wait for it to be started by yroom.broadcast_update() method. If yroom is started twice, an exception could be thrown and the exception could kill task groups. We need to add exception handling for ystore.start() if it get started in multiple places.
|
@Zsailer I don't think we should merge when there are requested changes that were not made, in my opinion. We should at least justify that these changes are not needed, but the best would be to wait for the reviewer's answer. |
This was my mistake, @davidbrochart, an oversight in my follow-up review. I'm so so sorry. I thought @jzhang20133 applied your suggestions after moving that chunk of code from the handlers to the document room. I now see that she dropped one of the lines. This one is on me for not flagging this during the review. I will take this as a note to slow down.
❤️ absolutely, @davidbrochart. And we cannot express enough how much we appreciate you for this. Please know that this wasn't an attempt to bypass or ignore your review. It was an oversight on my part, and I apologize for not giving your time+review the proper attention ❤️ |
Yes, you're right the call this out. It's not a good excuse, but helpful to be transparent here. This issue was blocking us from putting on a "real" system with real people testing it, so we're trying to get these libraries in a useable state ASAP (they are pretty brittle today when running in a cloud environment where RTC is most useful). In the long run, I hope this brings helpful and powerful feedback for the project. That said, we don't believe this gives us the right to bypass your reviews and I hope you don't feel that we're doing that. Please call it out again if you do. We want to get this right. |
|
No worries Zach and Jialing, I hope my message was not too dry. |
Together with jupyter-server/pycrdt-websocket#40, resolving jupyter-server/pycrdt-websocket#41