Cannot pass NULL to WriteFile() in lpNumberOfBytesWritten, should use JNA definitions

[rdiez]

While investigating issue #118, I realised that we should pass NULL to WriteFile() in lpNumberOfBytesWritten. According to the Microsoft documentation:

"Use NULL for this parameter if this is an asynchronous operation to avoid potentially erroneous results."

"If hFile was opened with FILE_FLAG_OVERLAPPED, the following conditions are in effect:"
[...]
"The lpNumberOfBytesWritten parameter should be set to NULL."

However, I did not manage to pass NULL with the current API definition.

I then realised that JNA provides its own definitions for all the stuff in WinAPI.java . JNA's WriteFile() definition seems to allow a NULL in that particular parameter.

Is there a reason why PureJavaComm duplicates all those JNA definitions?

[nyholku]

IIRC those were not available when PJC was implemented or I was not aware of them.

What prevented you from passing NULL?

[rdiez]

Passing null there made the application crash on my system.

I did not really investigate further, as I assumed that JNA is using IntByReference instead of int[] for a reason. I'm afraid I do know much about JNA yet.

I did comment out the "log = log && ..." line in the WriteFile() wrapper beforehand. That code is referencing nwrtn[0] without checking whether nwrtn is null. That could also be fixed.

There is no mention in the JNA documentation whether you are allowed to pass null in this scenario.

[nyholku]

The logging, if enable, for sure will crash and that should of course be checked inside WriteFile in WinAPI.

JNA works pretty simply: if the C-API takes a int* then you can use either int[] or IntByReference or even both in the Java signature and passing Java null should have the same effect as passing NULL in C.

[rdiez]

Could you add a check against null in the logging line, as mentioned above?

I'll test again whether passing null crashes on my machine.

The JNA library is probably maintaining the Windows API definitions and structures better than this project can. Have you got any interest in moving the code to use the JNA definitions instead? If so, any hints about how to achieve that maybe step by step? Or any gotchas to expect?

[nyholku]

Sure, I will add the null check.

True, JNA is probably maintaining their Windows API better that this project in larger picture. However the very small subset of Windows API that PJC uses is pretty stable and by now I believe battle tested so there is, in my evaluation, little to be gained and risks involved so I'm not going to change to JNA WinAPI unless there is compelling reason. One of the gotchas is the in my binding I've tried to use 'JNA Direct Mapping' extensively and I have not looked if JNA WinAPI does the same. This has a marked effect on performance.

And I repeat, we should concentrate and focus on specific know problems and try to find the root causes and not general code improvements or maintenance. Even if we trace the root problem to my WinAPI I think it is better to fix that hypothetical problem rather than risk changing all calls and throw away years of battle hardening.

[rdiez]

The reason why I am looking at such maintenance is because, after a couple of hours investigating, the cause of my problems is not obvious. We are talking here about JNA API calls full of details. Therefore, I suspect that something subtle is going on.

Such subtleties tend to come up when you polish the code. In my experience, things like adding asserts, improving error handling and detection, upgrading libraries and looking at compiler warnings do help. I have already caught a missing "throw" because of a source code warning. I would not downplay the importance of having clean, up-to-date code.

Besides, it is no fun working on old, suspect code, full of warnings like "Volatile array field". Take a look here:

https://www.javamex.com/tutorials/volatile_arrays.shtml

You can of course silent that warning, because having "volatiles" around gives you more confidence, and then the next one, like "Synchronization of non-final field". The code should still be OK, or should it? An alternative is not to look at the warnings and then miss an important one. In this way, at some point in time, you give up and live with an ugly work-around (like the long pause I have implemented in my application in the mean time).

[rdiez]

OK, I was confused. Passing NULL on this parameter does not cause a crash at JNA level.

There is a second "log = log && ..." statement after WriteFile() that did try to dereference nwrtn[0] too, and that was failing when nwrtn is null.

I would suggest the following changes to PureJavaComm:

1. Modify both "log = log && ..." inside the WriteFile() wrapper in order to cope with nwrtn being null, because passing null there is valid under some circumstances.

2. Actually pass null instead of port.m_WrN for this nwrtn parameter in src/jtermios/windows/JTermiosImpl.java, routine write(), because that is what the Microsoft documentation states that you should do when calling WriteFile() in overlapped mode.

That actually helps to understand the PureJavaComm code better, because you are no longer misled into thinking that port.m_WrN is actually used at that point. Only GetOverlappedResult() uses the value returned through this parameter.