Some fixes and improvements #19
Conversation
…p to debug specific chrome installations)
…eatance and message responses losing)
There was a problem hiding this comment.
Thanks for the PR! I added a few comments which I think need to be addressed before we can merge this.
I also have a general question - what was the breaking change in websockets 14.0 and how does this PR address it? I didn't look into that at all yet, so I'm not sure what to look for here.
BTW, would you mind listing your changes in CHANGELOG.md under the [Unreleased] section? Feel free to add your username at the end of each bullet point if you'd like credit.
| try : | ||
| _, stderr_bytes = await self._process.communicate() | ||
| stderr = stderr_bytes.decode()[:1000] | ||
| except Exception : | ||
| pass |
There was a problem hiding this comment.
is the try/except needed? when would this cause an exception?
if this is to handle self._process being None, we should check for that explicitly instead of doing try/except
There was a problem hiding this comment.
also, why are we truncating to first 1000 characters? maybe if anything getting the last 1000 characters would be more useful?
There was a problem hiding this comment.
about exception handling : communicate is hard method (it use system calls and can raise exception by much reasons), and raise exception in block that already process exception isn't good for me ... in this case we can live with original exception without output (and debug output separatly)
about output : I got first 1000 chars, because I don't expect here much output (we only started chrome here and don't pushed some commands) - it is trivial protection for memory overuse for strange cases (for example : when chrome executable file replaced with something)
| """ + | ||
| ("Browser error output:" + stderr if stderr else '') |
There was a problem hiding this comment.
thanks, I've been meaning to add something like this. very useful
| if isinstance(json, str): | ||
| # Some chrome versions return partitionKey as string | ||
| return cls(json, False) |
There was a problem hiding this comment.
just out of curiosity are there any docs on this?
There was a problem hiding this comment.
I didn't find something in docs, simple logged CDP message and got from practice usage of chrome >=129
| return self.websocket.closed | ||
| return (not self.websocket) |
There was a problem hiding this comment.
I see you added
websocket = Nonebelow, so this will work when we close the connection, but aren't there also other ways for the websocket to get closed? what if the browser closes the connection?
to handle all cases, we should probably do like
self.websocket is None or self.websocket.closed
(this applies to the other places you changed this as well)
There was a problem hiding this comment.
closed property removed in 14 and logic of websockets authors :
closed can be changed only on operations (send/recv) - here no any loop in websockets that check sockets in background
closed attribute can be filled only on operations, that already raise ConnectionClosedError
using of closed attribute after this exception is bad design for applications
Co-authored-by: Stephan Lensky <[email protected]>
| requires-python = ">=3.10" | ||
| requires-python = ">=3.9" |
There was a problem hiding this comment.
hey, this change needs to be reverted if you want this PR to get merged - I am sticking with >=3.10 for the X | Y union syntax for type annotations.
BTW, sorry for the delay in responding, I need to do some further research to understand the websockets==14.0 changes.
There was a problem hiding this comment.
Can I replace|with typing.Union ? I need 3.9 compatibility …
There was a problem hiding this comment.
Sorry, I recreated MR, (can't find way to change branch in already created MR), I can't rollback python 3.9 adaptation in main and created MR with other branch : #22
|
Pull request recreated (for merge from other source branch) : #22 |
I hope, that now we have place to collect fixes for nodriver.
I'm trying to stabilize it for flare-bypasser and am debug now an intermittent issue with CDP replies being lost - case when some call can be timeouted (navigate, screenshot), but internally looks like listening loop don't see answer for CDP command, but recv it when we start browser closing (by timeout = 60) ...
Here few fixes and improvements, that I need, and I think - they are useful for the project:
__count__for CDP message id generation (it is strange) and never drop it to zero - I'm putting this part in order to prevent the loss of answers due to repetitions of CDP message id.