Replace assertions with verifies in ocean.io.* #251

nemanja-boric-sociomantic · 2017-09-14T13:12:54Z

No description provided.

These were never compiled nor maintained.

nemanja-boric-sociomantic · 2017-09-14T13:14:54Z

src/ocean/io/select/client/FiberSelectEvent.d

 import ocean.core.MessageFiber;

+import ocean.core.Verify;


double import

nemanja-boric-sociomantic · 2017-09-14T13:15:39Z

src/ocean/io/select/client/FileSystemEvent.d

@@ -383,10 +384,10 @@ class FileSystemEvent : ISelectClient
    {
        foreach ( ev; this.fd.readEvents() )
        {
-            assert(ev.mask);
+            verify(ev.mask != 0);


Probably would be better off with ev.mask != ev.mask.init

nemanja-boric-sociomantic · 2017-09-14T13:16:30Z

src/ocean/io/select/client/SelectEvent.d

@@ -251,6 +248,7 @@ public class SelectEvent : ISelectEvent

    protected override bool handle_ ( ulong n )
    {
+        verify(this.handler !is null);


this one is added here, because it was in line 147, but never compiled (in contracts of the abstract method's implementations are not compiled).

nemanja-boric-sociomantic · 2017-09-14T13:19:17Z

src/ocean/io/stream/Data.d

@@ -450,7 +452,7 @@ class DataOutput : OutputFilter
        private final void eat (Const!(void)* src, size_t bytes)
        {
                auto count = output.write (src[0..bytes]);
-                assert (count is bytes);
+                verify(count == bytes);


Is there any difference in using is vs == for value types?

For structs only (as they may provide own opEqual)

codecov · 2017-09-14T13:19:25Z

Codecov Report

Merging #251 into v3.x.x will decrease coverage by 0.06%.
The diff coverage is 26.29%.

@@            Coverage Diff             @@
##           v3.x.x     #251      +/-   ##
==========================================
- Coverage   71.96%   71.89%   -0.07%     
==========================================
  Files         458      458              
  Lines       38798    38836      +38     
==========================================
+ Hits        27922    27923       +1     
- Misses      10876    10913      +37

nemanja-boric-sociomantic · 2017-09-14T13:19:32Z

src/ocean/io/stream/Iterator.d

@@ -58,6 +60,7 @@ package import ocean.io.device.Conduit : InputFilter, InputBuffer, InputStream;

 class Iterator(T) : InputFilter
 {
+        import ocean.core.Verify;


double import

gavin-norman-sociomantic

All looks ok to me. I agree with your comments.

david-eckardt-sociomantic · 2017-09-14T14:32:39Z

src/ocean/io/select/protocol/task/TaskSelectClient.d

@@ -161,9 +160,9 @@ class TaskSelectClient: ISelectClient
        try
        {
            this.task = task.getThis();
-            assert(this.task);
+            verify(this.task !is null);


In cases like this we have to be careful: verify(object !is null) does not call the object invariant while assert(object) does. Task doesn't have an invariant so it's fine here, but in general this change can silently drop a debugging aid.

So the better thing to do would be:

// old assert(object); // replace with verify(object !is null && object); // call invariant

No, only literally assert(object) works. It is a magic expression to call the invariant.

And assert(task) is perfectly fine here:

it will just crash on the next line in release build

it protects against being called from main thread context - not something that may not be noticed before production deloyment

david-eckardt-sociomantic · 2017-09-14T14:35:12Z

src/ocean/io/serialize/StructSerializer.d

@@ -206,7 +200,7 @@ struct StructSerializer ( bool AllowUnions = false )
    size_t transmit ( bool receive, S ) ( S* s, void delegate ( void[] data ) transmit_data )
    in
    {
-        assert (s, typeof (*this).stringof ~ ".transmit (receive = " ~
+        verify(s, typeof (*this).stringof ~ ".transmit (receive = " ~


Should this be s !is null?

There's an in contract here anyway :-)

nemanja-boric-sociomantic · 2017-09-14T14:58:38Z

src/ocean/io/Path.d

@@ -1460,11 +1462,11 @@ in
            switch (pattern[i])
                   {
                   case '[':
-                        assert(!inbracket);


I've grepped for in$ and this showed up as a remaining in contract. This feels very expensive to be left in release build. Should I just leave it here and use assert?

Is patternMatch a function that you could imagine being called in time critical code?

Puh, I guess you're right. It's still O(n), but let's KISS.

I would keep it nested inside {} block to make it easier to eventually reconsider and remove it if needed

nemanja-boric-sociomantic · 2017-09-14T15:00:48Z

Updated to address @david-eckardt-sociomantic 's comment and to move one debug block from in contract (it couldn't fail, it was just printout) to the body.

nemanja-boric-sociomantic · 2017-09-14T15:21:33Z

Updated.

mihails-strasuns-sociomantic

Some arguable comments from my side :)

mihails-strasuns-sociomantic · 2017-09-15T09:38:08Z

src/ocean/io/FilePath.d

    {
+        verify(dst !is null);


verify(dst.length > 0) makes more sense here

mihails-strasuns-sociomantic · 2017-09-15T09:39:19Z

src/ocean/io/Path.d

@@ -1460,11 +1462,11 @@ in
            switch (pattern[i])
                   {
                   case '[':
-                        assert(!inbracket);


I would keep it nested inside {} block to make it easier to eventually reconsider and remove it if needed

mihails-strasuns-sociomantic · 2017-09-15T09:48:46Z

src/ocean/io/model/SuspendableThrottlerCount.d

@@ -33,6 +33,7 @@ module ocean.io.model.SuspendableThrottlerCount;
 *******************************************************************************/

 import ocean.transition;
+import ocean.core.Verify;


I think all checks in this module actually should remain assert - they don't protect from any kind of memory corruption and relate to hot path in apps.

they don't protect from any kind of memory corruption

But they are valid sanity checks, which is the purpose of verify. I don't see any harm leaving them as verify.

This is what I think as well. Perhaps the trick is "hot path"?

No, purpose of verify is specifically sanity checking you can not abandon. So that when you build -release you get maximum performance possible while still not compromising the app. No more, no less.

In other words, "sanity check that is nice to have to catch weird app state but which won't help to keep the app working and won't prevent anything bad" is pretty much definition of assert use case.

Huh... this makes it a very tricky thing to decide, in many cases. A bug in this class, for example, could lead to seriously wrong app behaviour. Can those checks be abandoned?

Also: I was viewing this conversion process as allowing us to run release builds of D2 apps without losing any of the sanity checks that are in place right now but also without the risk of Errors being thrown. This is a somewhat different (and I would argue far more pressing) goal.

mihails-strasuns-sociomantic · 2017-09-15T09:50:22Z

src/ocean/io/select/client/EpollProcess.d

@@ -300,8 +302,8 @@ public abstract class EpollProcess

                if (pid == -1)
                {
-                    assert( errno() == ECHILD );
-                    assert( this.processes.length == 0 );
+                    verify(errno() == ECHILD);


either assert or enforce - either this is protecting from OS being broken or checking external state that is beyond programmer control

mihails-strasuns-sociomantic · 2017-09-15T09:51:32Z

src/ocean/io/select/client/FiberSelectEvent.d

    {
+        verify(super.fiber.waiting);


assert is fine, fiber will have own verify on resume

mihails-strasuns-sociomantic · 2017-09-15T09:59:22Z

src/ocean/io/select/protocol/task/TaskSelectClient.d

@@ -161,9 +160,9 @@ class TaskSelectClient: ISelectClient
        try
        {
            this.task = task.getThis();
-            assert(this.task);
+            verify(this.task !is null);


And assert(task) is perfectly fine here:

it will just crash on the next line in release build

it protects against being called from main thread context - not something that may not be noticed before production deloyment

mihails-strasuns-sociomantic · 2017-09-15T09:59:44Z

src/ocean/io/select/protocol/task/TaskSelectClient.d

            this.task.suspend();
-            assert(!this.task);
+            verify(this.task is null);


Also can be assert

mihails-strasuns-sociomantic · 2017-09-15T10:01:30Z

src/ocean/io/serialize/StringStructSerializer.d

@@ -219,7 +220,7 @@ public class StringStructSerializer ( Char )

    public void open ( ref Char[] output, cstring name )
    {
-        assert(this.indent.length == 0, "Non-zero indentation in open");
+        verify(this.indent.length == 0, "Non-zero indentation in open");


Error message is reverse wrong btw.

I think all those check must be assert (probably with reusable exception even) as serialization often happens with I/O data

It doesn't look that the error message is wrong :)

So many negatives :grumpy:

mihails-strasuns-sociomantic · 2017-09-15T10:03:56Z

src/ocean/io/serialize/StructSerializer.d

    {
+        verifyStructPtr!("dump")(s);


Same for this module - it all should enforce with actual exception, those are not sanity violations

mihails-strasuns-sociomantic · 2017-09-15T10:04:58Z

src/ocean/io/stream/Data.d

@@ -450,7 +452,7 @@ class DataOutput : OutputFilter
        private final void eat (Const!(void)* src, size_t bytes)
        {
                auto count = output.write (src[0..bytes]);
-                assert (count is bytes);
+                verify(count == bytes);


For structs only (as they may provide own opEqual)

mihails-strasuns-sociomantic · 2017-09-19T14:29:09Z

With last discussion in mind only remaining thing I'd like to happen is changing verify to enforce in serializer

nemanja-boric-sociomantic · 2017-09-19T14:41:48Z

I'll change them 👍

nemanja-boric-sociomantic · 2017-09-19T15:07:00Z

Updated with the enforcements for the StringStructSerializer and StructSerializer.

mihails-strasuns-sociomantic · 2017-09-20T09:11:37Z

Red

./src/ocean/io/serialize/StructSerializer.d(64): Error: constructor ocean.io.serialize.StructSerializer.SerializerException.ReusableExceptionImplementation!().this (ulong size = cast(ulong)128) does not match parameter types (char[],char[],int)
./src/ocean/io/serialize/StructSerializer.d(64): Error: expected 1 arguments, not 3 for non-variadic function type SerializerException(ulong size = cast(ulong)128)
./src/ocean/io/serialize/StructSerializer.d(64): Error: cannot implicitly convert expression (msg) of type char[] to ulong

Most serialization is being done with the external data, so in this case it's more appropriate to do enforce in this cases, with the reusable exception.

nemanja-boric-sociomantic · 2017-09-20T09:12:57Z

Updated.

nemanja-boric-sociomantic added 2 commits September 14, 2017 12:38

Remove debug (test) programs

69be238

These were never compiled nor maintained.

Use test instead of assert in unittest

62b74eb

nemanja-boric-sociomantic added this to the v3.5.0 milestone Sep 14, 2017

nemanja-boric-sociomantic requested review from gavin-norman-sociomantic, david-eckardt-sociomantic and mihails-strasuns-sociomantic September 14, 2017 13:12

nemanja-boric-sociomantic commented Sep 14, 2017

View reviewed changes

nemanja-boric-sociomantic force-pushed the verify branch from f32b2b4 to 4a96555 Compare September 14, 2017 13:26

gavin-norman-sociomantic reviewed Sep 14, 2017

View reviewed changes

david-eckardt-sociomantic reviewed Sep 14, 2017

View reviewed changes

nemanja-boric-sociomantic commented Sep 14, 2017

View reviewed changes

nemanja-boric-sociomantic force-pushed the verify branch from 4a96555 to eb8133e Compare September 14, 2017 15:00

nemanja-boric-sociomantic added 2 commits September 14, 2017 17:21

Change asserts to verifies

181c2db

Move debug block from in contract to body

2c7bf89

nemanja-boric-sociomantic force-pushed the verify branch from eb8133e to 2c7bf89 Compare September 14, 2017 15:21

gavin-norman-sociomantic mentioned this pull request Sep 15, 2017

Convert all assertions to verify #212

Closed

mihails-strasuns-sociomantic suggested changes Sep 15, 2017

View reviewed changes

Use enforce for verifications in ocean.io.serialize.*StructSerializer

006c22f

Most serialization is being done with the external data, so in this case it's more appropriate to do enforce in this cases, with the reusable exception.

nemanja-boric-sociomantic force-pushed the verify branch from 6229419 to 006c22f Compare September 20, 2017 09:12

mihails-strasuns-sociomantic approved these changes Sep 20, 2017

View reviewed changes

mihails-strasuns-sociomantic merged commit 1ff7eba into sociomantic-tsunami:v3.x.x Sep 20, 2017

nemanja-boric-sociomantic deleted the verify branch September 20, 2017 09:31

nemanja-boric-sociomantic added the type-refactoring label Sep 20, 2017

Replace assertions with verifies in ocean.io.* #251

Replace assertions with verifies in ocean.io.* #251

Conversation

nemanja-boric-sociomantic commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

gavin-norman-sociomantic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemanja-boric-sociomantic Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemanja-boric-sociomantic commented Sep 14, 2017

nemanja-boric-sociomantic commented Sep 14, 2017

mihails-strasuns-sociomantic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihails-strasuns-sociomantic commented Sep 19, 2017

nemanja-boric-sociomantic commented Sep 19, 2017

nemanja-boric-sociomantic commented Sep 19, 2017

mihails-strasuns-sociomantic commented Sep 20, 2017

nemanja-boric-sociomantic commented Sep 20, 2017

codecov bot commented Sep 14, 2017 •

edited

Loading

nemanja-boric-sociomantic Sep 14, 2017 •

edited

Loading