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

channel.close() less useful than thought #53

Closed
ghost opened this issue Jan 10, 2014 · 17 comments
Closed

channel.close() less useful than thought #53

ghost opened this issue Jan 10, 2014 · 17 comments

Comments

@ghost
Copy link

ghost commented Jan 10, 2014

Originally reported by: Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur)


Writing a unittest for sending a sequence through a channel, I tried the obvious, but failed:

#!python
    def testSequence(self):
        def sender():
            self.c.send_sequence(xrange(10))
            self.c.close()
        data = []
        def receiver():
            for i in self.c:
                data.append(i)

        stackless.tasklet(sender)()
        stackless.tasklet(receiver)()
        stackless.run()
        self.assertEqual(data, range(10))
        self.assertTrue(self.c.closed)


This doesn't work, however. The receiver is stuck on a iter.next call at the end.
This is because when the sender calls close, the channel balance is already -1. So it is set into 'closing' state, but that is it.
In order to do this properly, the functions needed to be tweaked like this:

#!python
        def sender():
            self.c.send_sequence(xrange(10))
            self.c.close()
            # this needs to change, close does not wake up a receiver, we must pump it
            while self.c.closing and not self.c.closed:
                self.c.send(None)

        def receiver():
            for i in self.c:
                data.append(i)
            #remove the extra "pump" nones at the end....
            while data[-1] is None:
                data.pop(-1)
            data.append(10)

I wonder if this system is how we want to do it? Wouldn't it be nicer if close() were immediate? It would wake up any blocked tasklets right away? We would get rid of the 'closing' attribute, or make it synonomous with 'closed'.

As it is, I don't really se a use case for the sequence protocol if it needs to be shored up in boilerplate anyway.

Is there a use case for the current closing/closed state transfer?


@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


Agreed. close() should be immediate IMO. Not that I've ever had the need to close a channel...

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


@ctismer
Any thoughts?
(I know that tagging Chris here curiously does not work, it seems like his username is somehow not usablle for anything but mail...)

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


It expanded. But I believe that Christian, like myself, gets email notification on all issue actions regardless of tagging.

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Interesting, maybe it is only my interactive web page that doesn't expand it in realtime. Same when trying to add him as a pull request reviewer...

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):


No, doesn't work me either. I've tried to search for variations on his name.

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Anonymous:


The behavior is exactly as I designed at that time, I remember that I had reasons
for that. Just that I can't remember...

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Anonymous:


@krisvale tagging did work nicely for me - it clearly tells be to be directly addressed.
But unfortunately only after I click a link in the notification email that I am getting
for all issue activities ;-)

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Well, We should probably try to dig out some use cases.
I understand how it is attactive to be able to say "do not add additional blocks" on the channel.
but the use case of iterating over a channel and getting a "StopIteration" immediately on close, if you are already blocked, is compelling.
If the old use case is still valid, and in use, we could achieve the new one with a "hard" flag, or something:

channel.close(hard=True) #close the channel and awake any waiting tasklets with an exception. next tasklets get StopIteration, others get ValueError.

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


(We didn't even have unittests for iter(channel) until today, I think)

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Anonymous:


Ah, I get closer to close()...
I wanted a way to have more control over channels and to make sure that no
more messages are transferred than intended.
It was not thought as to close something immediately, and I still think this way.
The channel should continue to work like normal.
The closing action is just what is planned, but I don't want to disturb normal action
of the channel.

Maybe the name of the function/flags was unfortunate. I did not want a close action
that kills the channel, what the proposed change would do.
I simply needed this:

  • I want to send 15 messages on this channel and then I'm done.

This is kind of future, and waking up anything was exactly not what I wanted.

I think the sequence protocol was added later, and it doesn't fit this case.

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


Right, but the sequence protocol is basically broken because of this, unless you explicitly send StopIteration at the end manually. Which is not really how you should do things. So, do we want to add a function, or a flag to a channel to properly support the iterator protocol?

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Anonymous:


I think the "closing" flag was just the idea to know that the channel will be closing.

Yes, let's improve that. When close is called, then this close call should

  • signal the fact that close was called

  • put something in the channel that ends the iterator.

Does that make sense at all, or should we throw that away, simply?

@ghost
Copy link
Author

ghost commented Jan 10, 2014

Original comment by Kristján Valur Jónsson (Bitbucket: krisvale, GitHub: kristjanvalur):


I have no idea :)
I never used the iterator until today. I wonder if anyone else is doing it. Is it even a good idea?
I've been playing around with the close flag for abit though. It looks useful. the use case I have in mind at the moment, I need to look at it again to see if would break with the immediate close semantics proposed....

In the end I suppose, it all depends on what people want, and what people are already using and comforable with. I propose we postpone this issue past 2.7.6 release.

@ghost
Copy link
Author

ghost commented Jan 11, 2014

Original comment by Anonymous:


The iterator thing was the hope to get some benefit from sending or receiving a bulk of data over a channel and to have a way to limit that. Not sure about it.

I added this when I used stackless for a larger project, myself. But that was 10 years ago.

Good that we have this as a dormant issue, at least.

@ghost
Copy link
Author

ghost commented Sep 4, 2016

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


This issue is related to #78. Therefore I tried the following variant of Kristjáns example.
It works. The only difference is the additional line self.c.send_exception(StopIteration)

#!python
    def testSequence2(self):
        def sender():
            self.c.send_sequence(xrange(10))
            self.c.send_exception(StopIteration)
            self.c.close()

        data = []
        def receiver():
            for i in self.c:
                data.append(i)

        stackless.tasklet(sender)()
        stackless.tasklet(receiver)()
        stackless.run()
        self.assertEqual(data, range(10))
        self.assertTrue(self.c.closed)

Therefore I propose to document, that

  • currently it is required to send the StopIteration manually
  • a future version of Stackless may create a StopIteration automatically (as proposed by Christian), if the channel gets closed.

Then I'll change the status of this issue to "on hold" and wait for an implementation.

@ghost
Copy link
Author

ghost commented Sep 5, 2016

Original comment by Anselm Kruis (Bitbucket: akruis, GitHub: akruis):


Documentation update is in 38b6cdef0178 for 2.7-slp.

@ghost
Copy link
Author

ghost commented Sep 5, 2016

Original comment by Christian Tismer (Bitbucket: ctismer, GitHub: ctismer):


After reading the full story again, I believe that your solution is good, and putting this into the example makes the people even aware of the problem!

@ghost ghost closed this as completed Sep 24, 2017
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

0 participants