-
Notifications
You must be signed in to change notification settings - Fork 1k
uasyncio: KeyError in remove_reader
(MacOS)
#248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
The backtrace you post don't seem to correlate with the code snippet you posted. You would need to review what callers of uasyncio do which causes such behavior. Alternatively, feel free to provide simple and self-contained example which reproduces this problem (directly against uasyncio or against other uasyncio-based reference software, like picoweb). |
Provided snippet is just for example of usage. However, original code is close to snippet. |
So I've got stable repro of this problem. #!/usr/bin/env micropython
import uasyncio as asyncio
def _handler(reader, writer):
print('New connection')
line = yield from reader.readline()
print(line)
yield from writer.awrite('Gotcha!')
yield from writer.aclose()
def run(host="127.0.0.1", port=8081, loop_forever=True, backlog=16):
loop = asyncio.get_event_loop()
print("* Starting Server at {}:{}".format(host, port))
loop.create_task(asyncio.start_server(_handler, host, port, backlog=backlog))
if loop_forever:
loop.run_forever()
loop.close()
if __name__ == '__main__':
run() This program is simply listen for TCP connection and then reads line.
$ nc -4 localhost 8081
^C
uasyncio is from commit d19253a which is last working version before breaking changes mentioned in #244 |
Forgot to mention, I'm using |
More investigations:
So fix could be pretty simple like: if id(sock) not in self.objmap:
return |
Thanks for preparing a testcase. But I can't reproduce the behavior you described, instead I get after pressing ^C on running
The script works pretty as expected. There definitely should be So, the first question you'd need to answer is why you don't get an EOF. What's a bit worrying is that writing to closed socket should lead to an error, and it doesn't happen. But "should lead" actually applies only to blocking socket and under some additional conditions. It's all more complicated with non-blocking sockets. Well, perhaps that matter worth being investigated (not directly related to this ticket). |
Yeah, good reasoning. Now you just need to explain how IOReadDone gets yielded. Nothing in the code you posted directly does that (e.g. writer.aclose() yields IOWriteDone). readline() may yield IOReadDone, but then why you don't get So, please keep investigating. You may need to go as deep as printing the values poll() returns on your system. I'm using Linux and the latest uasyncio for reference (and I recommend using the latter too). |
If you do that, please don't forget to post patch with print()s, I'll need to compare it with the values I get on Linux. |
remove_reader
remove_reader
(MacOS)
Whenever you P.S. Test case is slightly wrong - looks like I've attached first version of it and forgot to update. So to get it reproduced simply replace |
Hello, micropython-lib/uasyncio/uasyncio/__init__.py Lines 100 to 111 in f20d89c
If, for some reasons, a call to ios.read() doesn't return anything(this happens to me on a low memory situation), then you effectively have two calls to IOReadDone() -- one in .read(), the other in .aclose(). The second call will cause a key fault. An obvious fix is already in @belyalov's reply. |
In Python, a function always returns something.
Ok, that seems plausible.
That's obviously not consistent with how it's implemented for remove_writer(). |
Did you understand my post or did you have to go word-to-word? I said this happened in "a low memory situation." Socket read returns None because system is out-of-memory. Under such condition, there will be two calls to IOReadDone() in which the latter will cause a key fault. Next time, maybe try to pay attention to the actual code instead of people's words. |
There's such notion as "fail fast": https://en.wikipedia.org/wiki/Fail-fast . If you make mistake with such simple things as Python function returns, just imagine how many mistakes can be done with less obvious things?
Still not a correct analysis, see the code above.
As a maintainer of many (too many!) projects, I'm not interested in random quick-and-dirty "fixes", but in understanding the problem and finding the best solution, which is sometimes not obvious. I welcome anyone to apply the same approach and put the uasyncio code under scrutiny (but then the entire code, not just 2 random lines, it's a few screenfuls anyway). And if the description of a problem starts with "foo() doesn't return anything", please kindly let me be skeptical and call for more thoroughness on it. |
Please don't introduce irrelevant topic into this discussion. Thank you! Conclusion: |
@shawwwn I'd suggest you not to expect any patches accepted for Personally I decided not to spend any more time to talk to "maintainer" - simply because it is just time spending instead of problem solving. Fork it (or use existing one), apply patch and use it like I did. P.S. here is the fix: belyalov@68abd69 |
All fixes and further development happens in the upstream repo, https://github.com/pfalcon/micropython-lib . This issue will be fixed too, when someone prepares a proper patch for it. |
This remains the upstream library for the MicroPython project. That said, is this still an issue on recent versions on MicroPython and asyncio versions? |
uasyncio version 1.2.4
I'm using it in a pretty common way:
Sometimes (probably when connection is closed from client side) I see exception:
The text was updated successfully, but these errors were encountered: