Skip to content
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

unix/moduselect: poll.unregister: Add optional throw=True param. #4217

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@pfalcon
Copy link
Contributor

commented Oct 6, 2018

If 2nd, positional only argument of False is passed to poll.unregister()
method, then it won't throw exception if passed a stream object not
registered with the poller, which was the previous behavior. Then default
behavior is made compatible with CPython's, which is to throw exception
in this case.

pfalcon added some commits Oct 4, 2018

unix/moduselect: poll.unregister: Add optional throw=True param.
If 2nd, positional only argument of False is passed to poll.unregister()
method, then it won't throw exception if passed a stream object not
registered with the poller, which was the previous behavior. Then default
behavior is made compatible with CPython's, which is to throw exception
in this case.
tests/extmod/uselect_poll_basic: Test unregister() on stream not in p…
…oller.

After previous change, poll.unregister() is now by default compliant with
CPython behavior.
@pfalcon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2018

This is a follow-up to #4197, implementing handling as proposed in #1550 (comment)

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Making it raise a KeyError if the object is not found, to align with CPython behaviour, makes sense.

But it's not clear to me that unregister() needs to be extended to have the proposed "throw" argument. Divergence from CPython should be minimised where possible, and in this case it adds code to diverge. And uasyncio doesn't seem to need this behaviour since it already tracks what is in the poller with objmap.

@pfalcon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

But it's not clear to me that unregister() needs to be extended to have the proposed "throw" argument.

It adds idempotent behavior, which is always a useful feature.

Divergence from CPython should be minimised where possible, and in this case it adds code to diverge.

Well, everybody knows that, that's why I always followed "social contract" that we try to use existing Python features as far as possible, and if something missing, then extension is adding in a natural, generalizing way. Compare that again with your proposal to add adhoc functions vs my idea to stick with and reuse the standard Python formatting:
micropython/micropython-lib#77 (comment) .

Here, poll.register() provides useful idempotent behavior, and poll.unregister() accidental-gaply don't. So, we fix it in compatible, natural way.

And uasyncio doesn't seem to need this behaviour since it already tracks what is in the poller with objmap.

That's workaround which is supposed (and on the way) to go, per #1550 which talks about adding "callback" data to .register() call, to avoid double O(n) operations (where access to MicroPython dictionaries is O(n)).

I'd appreciate if this patch was merged soon, I'm using uasyncio in production, and now I get occasional crashes due to the changed behavior of .unregister(), so I intend to release new version of uasyncio using .unregister(x, False) soon.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Here, poll.register() provides useful idempotent behavior, and poll.unregister() accidental-gaply don't. So, we fix it in compatible, natural way.

It's still not clear it needs fixing at all. CPython seems fine without such a fix. uasyncio's use can be changed to something like this:

if objmap.pop(item, None) is None:
    poller.unregister(item)

And if that's not enough it can always be wrapped in a try/except block. Why add something that's not necessary and for which there is only one use case so for?

@pfalcon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

CPython seems fine without such a fix.

It's a weird argument to bring in. Turned out, that CPython is not fine without :=, while it (and everyone) was fine without it for 25 years. So what?

As I mentioned, objmap is a "dirty" workaround and supposed to go. Exceptions are expensive and allocate memory.

for which there is only one use case so for?

What do you mean "one usecase"? This usecase is idempotent operation.

Anyone can write an I/O scheduler, and there's high chance that such a feature will be useful, and there will be "many usecases". I don't know why everyone doesn't do that, but I indeed writing an I/O&Task scheduler which strives to be efficient, and number of changes to support that went into MicroPython (and only more in queue). That's "one usecase" which is supposed to enabled many-many usecases (application types which can be developed).

On top of that, I mentioned multiple times that I would like to finalize many tasks started, and this is one in the uselect module. The motivation is again: provide user with a freedom of choice of idempotent vs non-idempotent operation. In CPython, poll.register() is idempotent, poll.modify() is kinda non-idemponent variant, but poll.unregister is accidentally only non-idemponent. So, to close the gap, idemponent option is also provided, and there's usecase for it.

And there's no need to wait until that usecase hits hard, because then it will be preempted by much more substantial changes and cause unneeded havoc instead of sustainable development.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

I'm sorry, I don't agree with the above arguments. It's a functional addition that is not necessary. It's an optimisation (trading a small bit of Python code for some C code) which seems to have very little gain. It's not like it's a user-facing API improvement/optimisation because this stuff is very low level.

As I mentioned, objmap is a "dirty" workaround and supposed to go.

Then wait until that is improved before making changes like this, which may end up being temporary changes that are no used once the "bigger" improvement is made.

@pfalcon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

It's an optimisation (trading a small bit of Python code for some C code) which seems to have very little gain.

Unfortunately, we're past low-hanging optimizations which give big gain. Besides non-low-hanging big optimizations, there's also class of optimizations which you pinpoint well - those which low-hanging and provide "relative little" gain. But they still have their use: a) to clarify usecase/cover gaps; b) to uphold previously done optimizations. This is exactly one of them - the "service contract" for a scheduler is that it should not adversely affect the tasks running with it. A lot has went into making uasyncio scheduling not allocate memory by itself, and this is the change required to uphold that. So, even on "error paths" their should not be exception throwing, and this change is along that way. (Well, it just makes the existing behavior of such to blend with CPython's behavior with is not.)

@pfalcon

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

making changes like this, which may end up being temporary changes that are no used once the "bigger" improvement is made.

This is also pretty much how I feel (wary) about some of the further design and development to undertake. Bit again, unfortunately we're almost out of path which lead straight to the needed goal. You know that my approach was always on the side of "don't add stuff which may need to be removed" (ports, etc.) - but of course, that doesn't mean I wouldn't get into that situation myself even with all the caution I put into it.

It's better to cover the gaps and remove technical debt, even if something might need to be deprecated later. I'm still extra cautious about that stuff, I just have to accept that sometimes the best solution is not the single path, but adding choices/dichotomies. For example, I'm now almost sure that there should be parallel extended functionality added to both of memory vs stream buffers, so users have a choice for each particular situation, even if the selection ends up skewed in 90%/10% ratio.

So, here, I'm as the original designer/implementer of these things, sure that's the right addition.

And then there's a practical side - I'm happy to wait here, but I need to continue work and evolve on my side. Which means updating uasyncio. So, this is my move to avoid unneeded discrepancies for the community (community complained). And you make yours.

@pfalcon

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Some additional insight in the change made here: it has the same purpose as:

  • .get() method of dictionaries comparing to just subscribing [] - avoiding KeyError exception and returning None (or other default).
  • default param to dict.pop(key, [default]) - again, avoiding exception, returning a value instead.

Here, poll.unregister() is also desired to not raise exception, but as the method itself doesn't return anything, the added param is boolean throw, instead of default.

@traverseda

This comment has been minimized.

Copy link

commented Feb 23, 2019

First of all, thank all of you for your volunteer work on open source. It's amazing that I can even run python on microcontrollers, and it's opened up some new avenues for the commercial makerspace I'm working with.

I'm told that pfalcon/picoweb#45 is your fault. I'd appreciate it if you guys fought this out, because right now when I google "micropython web framework" and try to run the first example, it outright crashes. That's embarrassing, and makes micropython look like an immature platform.

I'd like to be able to offer kits based around micropython, but this is the kind of thing that causes people to tell me we should just stick with arduino. I don't know where or how this should be fixed, but I do think you should spend some more time getting this sorted out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.