Skip to content

Remove redundant writer.stop call that throws error #1317

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

Merged
merged 3 commits into from
Jun 1, 2022

Conversation

j-c-cook
Copy link
Contributor

The issue is that the writer has already previously been told to stop.

The function on_message_received is called:

python-can/can/io/logger.py

Lines 198 to 206 in 796b525

def on_message_received(self, msg: Message) -> None:
"""This method is called to handle the given message.
:param msg:
the delivered message
"""
if self.should_rollover(msg):
self.do_rollover()
self.rollover_count += 1

Which calls the self.do_rollover() function

python-can/can/io/logger.py

Lines 333 to 335 in 796b525

def do_rollover(self) -> None:
if self.writer:
self.writer.stop()
. The writer is told to stop:

    def do_rollover(self) -> None:
        if self.writer:
            self.writer.stop()


        sfn = self.base_filename
        dfn = self.rotation_filename(self._default_name())
        self.rotate(sfn, dfn)


        self._writer = self._get_new_writer(self.base_filename)

The _get_new_writer function then tells the _writer to close

python-can/can/io/logger.py

Lines 219 to 221 in 796b525

# Close the old writer first
if self._writer is not None:
self._writer.stop()
.

        # Close the old writer first
        if self._writer is not None:
            self._writer.stop()

I believe writer and _writer are linked. So the call to close the _writer is _get_new_writer is redundant and is throwing an error because the writer is no longer open.

closes #1316

The issue is that the writer has already previously been told to stop.
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #1317 (9ebdbb8) into develop (4cb2f2f) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1317      +/-   ##
===========================================
- Coverage    65.98%   65.97%   -0.01%     
===========================================
  Files           86       86              
  Lines         8948     8946       -2     
===========================================
- Hits          5904     5902       -2     
  Misses        3044     3044              

@zariiii9003 zariiii9003 added bug file-io about reading & writing to files labels May 26, 2022
@zariiii9003 zariiii9003 added this to the 4.0.1 milestone May 26, 2022
j-c-cook added 2 commits May 26, 2022 17:38
The _get_new_writer function was previously calling
`self._writer.close()`. The `self.writer` function is already being
closed in the call stack. Due to `self.writer` and `self._writer`
being linked, there was an error that the IO file was closed.

The `self._writer.close()` was commented out in a prior commit as a
proposal for the change. That comment is now removed. The docstring
is updated.
@j-c-cook j-c-cook requested a review from zariiii9003 May 26, 2022 23:57
@j-c-cook j-c-cook marked this pull request as ready for review May 26, 2022 23:57
@zariiii9003
Copy link
Collaborator

@felixdivo could you take a look? Can we remove it?

@j-c-cook
Copy link
Contributor Author

j-c-cook commented May 31, 2022

@zariiii9003 Please note that @felixdivo has posted several comments documenting that he is currently unavailable to contribute to the project. (#965 (comment), #1220 (comment)).

I have verified that txt, log, asc, csv and blf all work with the line I removed. However, I have not yet post parsed the output logs to ensure the signal being sent is captured.

Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

Yes, this looks good to me. 👍 😄

@zariiii9003
Copy link
Collaborator

Yes, this looks good to me. 👍 😄

Thank you! I thought maybe i missed some edge case where it is necessary.

@zariiii9003 zariiii9003 merged commit 47f673b into hardbyte:develop Jun 1, 2022
@j-c-cook j-c-cook deleted the issue1316_blfLogError branch June 8, 2022 21:28
@felixdivo felixdivo modified the milestones: 4.0.1, 4.1.0 Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug file-io about reading & writing to files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in can.logger during rollover when file format .blf is defined
3 participants