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

Update dhcp.py #8

Merged
merged 1 commit into from
Dec 28, 2018
Merged

Conversation

vitormhenrique
Copy link
Contributor

@vitormhenrique vitormhenrique commented Dec 28, 2018

Fix issue where DelayWorker would keep the server from closing if the queue is empty (issue #7).

Fix issue where DelayWorker would week the server to close if the queue is empty.
else:
func(*args, **kw)
if not self.queue.empty():
p = self.queue.get()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the queue is empty, this leads to a while loop which powers through. I think, we can use an else: here which sleeps for 0.01 seconds.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to add that?

Copy link
Collaborator

@greggzj greggzj Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, great thanks for your great library.
Just have two points for this ISSUE( though it already closed)

  1. what if using EAFP! style and adding timeout=1 rather than using LBYL style and directly continue the while loop without any delaying or sleep?
    The updated code will look like this:
try:
                p = self.queue.get(timeout=1)
                if self.closed:
                    break
                t, func, args, kw = p
                now = time.time()
                if now < t:
                    time.sleep(0.01)
                    self.queue.put(p)
                else:
                    func(*args, **kw)
            except queue.Empty:
                continue
  1. Do you think new release should be issued? I download the latest RELEASE version seems do not contain this fix

What do you think? Thanks in advance

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @greggzj, (1) you can open a pull request with the changes you would like to see and we can discuss them there.
(2) I also think that this fix is not yet in a new release. However, it is in the master branch which you can download, too. Yep, I make a new release.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will create a pull request later. Also thanks for the new release

@niccokunzmann niccokunzmann merged commit 58e9b01 into niccokunzmann:master Dec 28, 2018
@niccokunzmann
Copy link
Owner

Thanks for taking care of issue #7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants