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

Introduce WriteAsync to not swallow all exceptions and improve write performance and misc enhancements #130

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

panot-hong
Copy link
Contributor

🚨 Issues found in the current version.

  1. The current version when call Write...
    • It swallows all exceptions.
    • The caller is impossible to know when the bytes are written to the stream. It could be in the next couple of hours when the connection is restored which it may no longer need.
  2. It works well only when declaring the printer object as a singleton. Create a printer object multiple times causes the later ones do not really print when calling Write. That is due to the TCP connection does not properly manage to dispose but await for GC to collect the former one(s) so the printer remains to connect to the former connection. The new printer object performs write to the stream that no one listening.
  3. WriteLongRunningTask remains running forever even the printer object is disposed because its CancellationTokenSource is never canceled.
  4. NetworkPrinter creates infinite new tasks while attempt to connect.
  5. Looking at the code of SuperSimpleTcp, calling ConnectWithRetries is a little more expensive than just call Connect. With the fact that less percentage of the NetworkPrinter to fail to connect since the first time, I assume trying to call Connect as the first attempt should give slightly better performance.

✨ Purpose changes in this PR

  1. Introduces the WriteAsync function to allow using await or Wait() to explicitly capture the exception if it takes longer than the configurable timeout to connect prior. The legacy Write function remains to use as-is for swallowing exceptions and no need to wait for the connection. Calling WriteAsync without await nor Wait() behaves the same as calling the legacy Write function (this purpose to fix 1 above).
  2. Update TCPConnection to implement IDisposable and dispose TCPConnection in NetworkPrinter's dispose function.
  3. Replace ConcurrentQueue of writing by marshaling byte array with BlockingCollection and TaskCompleteSource. This made WriteAsync possible and remain legacy behavior with Write or WriteAsync without awaiting yet also enhance speed 🐎 to the WriteLongRunningTask (compare result by running ConsoleTest + TCPPrintServerTest). This also fixes issue 3 above.
  4. Fix issue 4 above.
  5. Revise per issue 5 above.
  6. Update ConsoleTest to be able to test using singleton printer object or short-lived printer object with a single bool variable switch.
  7. Enhance TCPPrintServerTest to support multiple TCP connections. This is for both when the same TCP connection is reconnected or when the TCP connection is closed and then create a new TCP connection.

Sorry to pack many changes 📦📦 in a single PR 🙇

@lukevp
Copy link
Owner

lukevp commented Sep 25, 2021

Thanks @panot-hong for the PR! I will need to take some time to digest the changes and fully test, as the various failure scenarios are quite extensive with this library so it's quite difficult to uniformly validate the behavior (which in itself hints that we need a more exhaustive test harness, especially for TCP printers, but also in general), but I'll do a review when I can.

I have some questions about your general approach though. It seems in this new model within this PR, if WriteAsync is using a blocking collection, and your underlying printer is not connected or in a bad state (eg. it has the cover open) the writes will block and timeout. So there's no mechanism to allow the writes to queue up if you want them to, which you can do with the current implementation. So while I agree with you that there should be a way to await a write request, I do think that there could be a way to support both patterns.

What do you think about having the concept of each write request having an ID that you can then query to determine if it is written or not? That way, you could still const id = Write(bytes) without any delay (or maybe it's called EnqueueWrite() to be more accurate), and you could await WaitForPrintToCompleteAsync(id) if you want to block until the write is complete? Then you'd also be able to GetCurrentPrintJobs() and use that to display queue information in a UI, for example.

Do you have something like this in your application (that manages print jobs, and knows if they have gone through or not / what the status of them is), or do you just do a blocking write every time you want to print, and if it doesn't get written to the underlying stream in the right amount of time you just throw errors and start over?

I think this is an important conversation to have, because it sounds like you use the library primarily for making throwaway printer objects that you only use once, where others use it for, as you said, a single instance per printer (not really a singleton, but a 1:1 of physical printer to printer object, and they are long-lived). In this scenario the printer object is sort of a software-level mock of the hardware. This is also a beneficial pattern if you have a UI that shows information about the printers, because it can get automatic status back messages which allow you to reflect the printer state in real-time without having to poll the printer. I think this is a very powerful benefit to keeping a long-running reference to a printer in the application.

@panot-hong
Copy link
Contributor Author

panot-hong commented Sep 26, 2021

Thank @lukevp for considering my PR.

I have some questions about your general approach though. It seems in this new model within this PR, if WriteAsync is using a blocking collection, and your underlying printer is not connected or in a bad state (eg. it has the cover open) the writes will block and timeout. So there's no mechanism to allow the writes to queue up if you want them to, which you can do with the current implementation. So while I agree with you that there should be a way to await a write request, I do think that there could be a way to support both patterns.

I want to iterate that my PR does not tend to replace the existing functionality but to also supports awaitable write (not swallow exception) plus better performance. I would separate this into two main things.

  • BlockingCollection, was not added only for WriteAsync but the main purpose is for better marshaling passages since the original code using producer/consumer approach by enqueue to ConcurrentQueue as a producer and while true polling and task sleep as a consumer. This is what BlockingCollection exists for without a need of a task wait. This gives a better performance in general. This can be tested and compare the result using ConsoleTest + TCPPrintServerTest. No need to use benchmark lib, the diff is up to a second+ for large bytes printing options in ConsoleTest. This BlockingCollection works well with all scenarios - the existing Write function, WriteAsync without await, and WriteAsync with await. In other words - it is just a replacement of ConcurrentQueue + CancellationTokenSource (which the current code never cancels).
  • TaskCompletionSource, is the actual actor that enables awaitable write. Even without BlockingCollection, I can inject TaskCompletionSource along with the ConcurrentQueue and perform the same as the rest of this PR.

I however just realized that the original PR missed retrying for the existing write function in the WriteLongRunningTask function. So I made a small change and commit to this PR to endlessly retry write to the stream (as long as the object is yet disposed) if it is bytes that submitted via Write function, not WriteAsync. With this, I recommend renaming the Write function to something like "WriteWithRetries" or "EnqueueWrite" like you said to be more explicit to its behavior. But if you mine backward compatibility - remain the same name and adding an explanation to the XML doc and README should also help.

What do you think about having the concept of each write request having an ID that you can then query to determine if it is written or not? That way, you could still const id = Write(bytes) without any delay (or maybe it's called EnqueueWrite() to be more accurate), and you could await WaitForPrintToCompleteAsync(id) if you want to block until the write is complete? Then you'd also be able to GetCurrentPrintJobs() and use that to display queue information in a UI, for example.

I think the WriteAsync also does pretty similar to this. As a common usage of the task, as long as it is not awaited, it is pretty much like EnqueueWrite(). And once it is awaited, it is WaitForPrintToCompleteAsync(id).
Task t = WriteAsync(bytes); // same as EnqueueWrite()
// do something...
await t; // same as WaitForPrintToCompleteAsync(id)

Do you have something like this in your application (that manages print jobs, and knows if they have gone through or not / what the status of them is), or do you just do a blocking write every time you want to print, and if it doesn't get written to the underlying stream in the right amount of time you just throw errors and start over?

I am not at that stage yet but quite certain that there will be and that is one of the reasons I decided to use this library in the first place. Again, this PR does not limit to only use blocking (await), the existing Write function should work properly as it was. Also, calling the await WriteAsync(bytes); does not block you from doing what the Write function does. Wrapping WriteAsync within a try-catch should give the caller direct insight into whether the bytes were passed through or not and what to do with that bytes if fail to write, retry or abort or showing a dialog to let the user decide. It is just an option to give the full control by the caller.

I think this is an important conversation to have, because it sounds like you use the library primarily for making throwaway printer objects that you only use once, where others use it for, as you said, a single instance per printer (not really a singleton, but a 1:1 of physical printer to printer object, and they are long-lived). In this scenario the printer object is sort of a software-level mock of the hardware. This is also a beneficial pattern if you have a UI that shows information about the printers, because it can get automatic status back messages which allow you to reflect the printer state in real-time without having to poll the printer. I think this is a very powerful benefit to keeping a long-running reference to a printer in the application.

You are right, the most use case should be where a thermal printer is assigned to a device. But in some scenarios like when a thermal printer is set up in a kitchen as a kitchen printer and having multiple POS devices that write stream to the single kitchen printer. The TCP connection to the printer when one is currently connected, another one has to wait for the first connection to close. In this kind of scenario, the caller wouldn't mind tracking printer status and should release the connection right after the bytes are written to the stream.
Again, WriteAsync is not necessary to use with the short-lived print object. Even the long-lived print object can use try-catch with WriteAsync, it is just a matter of not swallowing the exception and the caller knows exactly when the bytes are written to the stream.

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.

3 participants