Skip to content

Conversation

@hardbyte
Copy link
Owner

@hardbyte hardbyte commented Sep 8, 2018

Competes with #409 because I forgot to look if you were already doing this @christiansandberg !

  • adds task management responsibility to the Bus. By default tasks created with bus.send_periodic will have a reference held by the bus - this means in many cases the user doesn't need to keep the task in scope for their periodic messages to continue being sent. If this behavior isn't desired pass store_task=False to the send_periodic method.
  • the bus now has a stop_all_periodic_tasks method
  • The socketcan tasks now reuse a bcm socket

can/bus.py Outdated
self._lock_send_periodic = threading.Lock()
return ThreadBasedCyclicSendTask(self, self._lock_send_periodic, msg, period, duration)
task = ThreadBasedCyclicSendTask(self, self._lock_send_periodic, msg, period, duration)
self._periodic_tasks.append(task)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We’ve discussed this before but this can potentially become a memory leak. There is not way for the user to clean out stopped tasks. I don't know what to do about it. Maybe it could be possible to remove them if they are stopped, because then we know that the application has its own reference to the task.

The kernel's broadcast manager will be used.
The kernel's broadcast manager will be used and the task will
be active while this Bus instance is still in scope.
Copy link
Collaborator

Choose a reason for hiding this comment

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

...or shutdown() or stop_all_periodic_tasks() is called?

@felixdivo
Copy link
Collaborator

Note: We might also consider calling shutdown() in the __del__ method of the bus.

@codecov
Copy link

codecov bot commented Sep 10, 2018

Codecov Report

Merging #412 into develop will increase coverage by 0.91%.
The diff coverage is 68.57%.

@@             Coverage Diff             @@
##           develop     #412      +/-   ##
===========================================
+ Coverage    61.51%   62.43%   +0.91%     
===========================================
  Files           55       55              
  Lines         4584     4600      +16     
===========================================
+ Hits          2820     2872      +52     
+ Misses        1764     1728      -36

@hardbyte
Copy link
Owner Author

Hmm okay what about this slightly hack approach of wrapping the task.stop() method inside bus.send_periodic. There would still be issues with resumable tasks and expired tasks though.

@felixdivo felixdivo added the bug label Sep 11, 2018
@felixdivo felixdivo added this to the 2.3 Release milestone Sep 11, 2018
@hardbyte
Copy link
Owner Author

Regarding the memory leak issue at the least we should clearly document what the user can do to prevent it, but I'd rather have an api that allows the user to take ownership for the task management if they want to. Perhaps a flag in the bus initializer or the send_periodic method?

@hardbyte hardbyte force-pushed the feature-socketcan-bcm-owned-by-bus branch from d342f46 to 2e734b2 Compare September 17, 2018 02:41
can/bus.py Outdated
self.shutdown()

def __del__(self):
self.shutdown()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not all implementations may be able to handle shutdown being called twice.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fair point - I'd rather remove the __del__ from the bus instead of adding shutdown state tracking. Do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so too.

@hardbyte hardbyte force-pushed the feature-socketcan-bcm-owned-by-bus branch 2 times, most recently from 5189764 to 76dc836 Compare September 22, 2018 00:17
@hardbyte hardbyte force-pushed the feature-socketcan-bcm-owned-by-bus branch from 76dc836 to 4660091 Compare September 22, 2018 00:40
@hardbyte
Copy link
Owner Author

hardbyte commented Sep 22, 2018

I think this is pretty much ready - I've just rebased.

The last change I made - added a remove_tasks parameter to the stop_all_periodic_tasks method which calls the wrapped stop method.

@felixdivo felixdivo merged commit 8d92a05 into develop Sep 23, 2018
@felixdivo felixdivo deleted the feature-socketcan-bcm-owned-by-bus branch September 23, 2018 14:39
@felixdivo
Copy link
Collaborator

I just merged this. Someone else (@christiansandberg ?) needs to have a look at #292 and #404.

@hardbyte hardbyte mentioned this pull request Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants