Clean up object disposal and shutdown long running tasks.

[hollandar]

Hi,

This PR makes changes to the base printer object, and the serial and network variants, and to the Esc-Pos protocol.
We have been integrating this on dotnet core across multiple platforms, and had a few challenges.

Base Printer

• Printer types did not dispose consistently for device testing purposes, mostly because the long running threads did not shut down totally. In the case of the serial printer, the read thread was being blocked by the serialport.read call.
• Removed the binary reader and writer objects and instead move reading and writing down to the printer object itself, which allows the serial printer version to only read if there are bytes available.
• NetworkPrinter was executing a thread that would continually try to reconnect even if the printer connected to was no longer used, which stopped it from being disposed.

EscPos protocol.

• There are alternate versions of the alignment commands depending upon the version of the printer you have.

Thanks for your component, it works with Epson printers nicely.
Adrian

[lukevp]

Hi @hollandar glad you like the library! This PR is too big and impactful to merge without some significant review and discussion. Part of the changes introduced in 2.0 were to allow the network printer to manage connectivity automatically, for example, but this PR reintroduces the concept of manually having to connect. So I don't think this PR matches with my philosophy of an ESCPOS object = a connection to a printer and the library hides the intricacies behind that from you. Did you attempt to update the dispose logic to properly dispose of objects instead of changing the connection management? Do you have a test case that easily shows this disposal issue? Having said that, this PR and #130 both address the implications of the 2.0 changes, so I do see this as a problem and I'd love to fix it, but I think it makes sense for the library to manage connectivity and monitoring automatically for the end user so I'd rather fix it via properly managing and disposing rather than requiring some additional interaction with the library to manage the connection lifecycle manually.

Could you make a separate PR with the escpos protocol changes? That one I can merge with little review, and we can iterate on this PR instead.

[hollandar]

Thanks Luke,

I'll enumerate the problems we found as a way to start discussing how to address the issues.

1. Connection to a network printer succeeds even if the printer isn't there because the exception is swallowed in AttemptReconnectInfinitely.
2. Destruction of any printer does not complete because _writeCancellationTokenSource?.Cancel(); is never called leaving the long running thread active. This is true regardless of the connection state.
3. Destruction of the network printer never occurs if the initial connection failed because AttemptReconnectIndefinitely continues to respawn itself.
4. Printers block at var b = Reader.BaseStream.ReadByte(); ( BasePrinter.cs 144); this usually is a blocking call that waits for bytes to become available. The long running reader thread continues regardless of the cancellation token being signaled because it never tests the token as it is blocked at this point. One simple solution to this is to ReadAsync from the stream at that point and pass the read cancellation token to it.

I dealt with these by:

1. Performing the connection in the Connect method, explicitly called from consumer code, and no longer swallowing the exception.
2. Call _writeCancellationTokenSource?.Cancel() on destruct of the base printer.
3. Remove attempt reconnect indefinitely so the exception is thrown on any reconnect.
4. Delegate reading the stream to the actual printer object rather than going through the BinaryReader/BinaryWriter so byte availability checks can be handled appropriately per device.

I hear you on the 2.0 changes; but unsure how you address it.

This PR surfaces the NetworkPrinter failure on the first time around, and subsequently reconnects after disconnections with a timeout; not silently and not forever. In our testing so far handling the disconnected event has been enough to keep the printer connected; are there scenarios you were addressing in 2.0 that this approach didn't address??

I did attempt to stop attemptreconnectindefinitely from occurring on disposal by exposing the write cancellation token to NetworkPrinter but this didn't feel right.
I also tried raising a cancellation token in overridabledispose on the network printer which works, but that does mean that if the printer has actually gone away you don't ever discover that it is gone nor why; other than it no longer prints.

Adrian.

[jkatsiotis]

Hi @hollandar I am incorporating the lib into an enterprise app and I've faced all the problems you are mentioning. While this lib by @lukevp is a great starting point it cannot be used in production environments without addressing theses issues first. Should I head over to your repo and maybe contribute there? Are you actively developing this?

Thanks

[hollandar]

Hey John, We are running the version in my fork in production on Linux with test environments on Linux and Windows. Our printers are TM-20’s so I have not extensively tested the straight serial components on Linux, primarily TCP.  Since we are running my fork in prod, happy to take PR’s on it; we will come back around to it in a second pass later this year.  Cheers, Adrian. 

[jkatsiotis]

I'm also using TCP and mixing EPSON with CITIZEN printers. If anything comes up I'll write/ping you on your repo

[lukevp]

@hollandar I think something went south with your recent comment, may want to clean it up. :)

@jkatsiotis, I hear you on these issues. I'm sorry it's causing you trouble. I've had a lot going on personally/professionally in the past few months, but I want to continue to support this library. I'll set some time aside this month to review all open PRs, incorporate changes where it makes sense, and cut a new release. I think the 2.0 move to persistent connections was not an ideal solution based on the issues that have come in. Sounds like you can use @hollandar 's fork for now but I hope I can win you back to the OG repo once these changes have been made.

Just as a general note, I don't use this library myself in production, so I'm super appreciative of the PR and the contributions. I just need to make sure they all work together in a sensible way as there are multiple competing philosophies on how the library should function, so I have to conform these to ensure the overall project goals are maintained. I think we're getting enough users that there should be a roadmap of future work and changes that would really help everyone get aligned on where this lib is going. I'll do that as well.